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 MeshLibrary and GridMap change notifications #76468

Closed

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented Apr 26, 2023

Fixes MeshLibrary and GridMap change notifications.

Fixes a part of #76429.

Properly other older bug reports that had issues with Editor GridMap palette updates as well.

Fixes MeshLibrary and GridMap change notifications.
@smix8 smix8 requested a review from a team as a code owner April 26, 2023 09:14
@akien-mga akien-mga added this to the 4.1 milestone Apr 26, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Jun 11, 2023

Doesn't seem enough to close the issue:

godot.windows.editor.dev.x86_64_ZJyT11LZVE.mp4

The preview is not updated when material changes.

@smix8
Copy link
Contributor Author

smix8 commented Jun 11, 2023

Not sure how to handle this. The palette used fixed images that can be customized by the user or not, else the preview property wouldn't be exposed on the MeshLibrary item. But there is no distiction between the auto-generated preview and a user defined custom preview. So we can't just auto-update the preview on (sub) resource changed signals cause then any user custom preview gets replaced by a generic preview again.

@YuriSizov
Copy link
Contributor

We could store them separately internally, could we not? And return the auto-generated one if user never set a custom one. The signal can be fired whenever any one of them changes, it shouldn't be a problem.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 11, 2023

On a side note, I don't know who thought that storing the preview inside the library is a good idea. The image is getting embedded in the resource. If you inspect the library from the MRP, 90% of its content is binary data saved as text.

I wonder if users really use this feature for custom previews, or if we could just make this property not stored. Alternatively we could add a new property like custom_preview, which decides whether preview is saved or not.

@smix8
Copy link
Contributor Author

smix8 commented Jun 21, 2023

Reacting to changes in the subresources revealed that there is a problem with a cascade of update trigger by it. I think this should be left for another pr because so far I don't even know what parts of the Editor trigger all those extra updates. Same as with changing the preview image storage.

@YuriSizov
Copy link
Contributor

Reacting to changes in the subresources revealed that there is a problem with a cascade of update trigger by it.

What does it look like? What's the impact on the editor or the user?

@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 23, 2023
@smix8 smix8 closed this Aug 19, 2023
@smix8 smix8 deleted the meshlib_and_gridmap_notify_4.x branch August 19, 2023 17:40
@AThousandShips AThousandShips removed this from the 4.2 milestone Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants