Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MissingResource properties being stripped on save #86600

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

nikitalita
Copy link
Contributor

fixes #86599

See #60597 (review)

There were a number of bugs in the MissingResource implementation that prevented it from actually preserving the missing resources when saving; I have corrected these.

@fire
Copy link
Member

fire commented Feb 12, 2024

@reduz Preserving the missing resources when saving is a good thing, but I would like a technical second look.

@fire
Copy link
Member

fire commented Mar 11, 2024

Can you do a rebase? I'll try to review.

@nikitalita nikitalita force-pushed the fix-missing-resources branch from 6fa3ea9 to 87cce79 Compare March 12, 2024 01:57
@nikitalita
Copy link
Contributor Author

Thanks, I've rebased on current master

@akien-mga akien-mga modified the milestones: 4.3, 4.4 Jun 18, 2024
@nikitalita nikitalita force-pushed the fix-missing-resources branch from 87cce79 to 2e7d923 Compare September 30, 2024 08:29
@nikitalita nikitalita requested a review from a team as a code owner September 30, 2024 08:29
@nikitalita
Copy link
Contributor Author

@fire I've rebased again, can you take a look?

@nikitalita nikitalita force-pushed the fix-missing-resources branch from 2e7d923 to 5ab8636 Compare October 14, 2024 09:57
@nikitalita nikitalita requested a review from a team as a code owner October 14, 2024 09:57
@nikitalita nikitalita force-pushed the fix-missing-resources branch from 5ab8636 to e338945 Compare October 14, 2024 10:13
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok, but it's also touching core so I'm a bit wary.

Will try it on my projects.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style looks good

@nikitalita nikitalita force-pushed the fix-missing-resources branch from 23f5f4a to 95d2909 Compare November 5, 2024 17:54
@hpvb
Copy link
Member

hpvb commented Nov 5, 2024

LGTM

@lyuma
Copy link
Contributor

lyuma commented Nov 5, 2024

The bug and this PR are fixing an issue "stripped upon save". Why then is it modifying load-side functions ResourceLoader and scenestate::_parse_node?

i am concerned that this implies some form of compat breakage (resources changing next time I update the engine)

@nikitalita
Copy link
Contributor Author

The bug and this PR are fixing an issue "stripped upon save". Why then is it modifying load-side functions ResourceLoader and scenestate::_parse_node?

i am concerned that this implies some form of compat breakage (resources changing next time I update the engine)

Please see here for a full explanation, but basically:

The reason for ResourceLoader being modified is that the pre-existing missing resource assignment was buggy; thus, the missing resources were not actually being loaded as MissingResources, and as such were not being saved when the scene/resource was re-saved.

The reason for scenestate::_parse_node being modified:

Nodes report their theme override properties as having PROPERTY_USAGE_STORAGE based on whether or not they're actually set. If a theme override property was set to a missing resource, it will be reported as not having PROPERTY_USAGE_STORAGE and it will be skipped. This should check if missing_resource_properties has the property.

Basically, when saving a scene, nodes that had properties set to a missing resource were skipped because they weren't previously loaded into the packed scene.

This shouldn't mess with scene instancing. During actual instancing, when attempting to assign a MissingResource to a node property, it'll just silently fail.

@Repiteo Repiteo merged commit 7d31e16 into godotengine:master Nov 10, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 10, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Resources with properties set to missing resource types are stripped upon save
8 participants