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

Avoid bubbling up changed notification when on new item/key of dictionaries to avoid inspector wipeout. #96623

Conversation

ajreckof
Copy link
Member

@ajreckof ajreckof commented Sep 5, 2024

fix #95647
This fix the issue at hand but first of all I'm not sure this is the right fix second I'm not even sure this property should be exposed in the inspector as it doesn't seem to be the right way to edit it (dedicated interface in the animation bottom panel). In any case this fix is safe so it can be included anyway.

@fire fire changed the title Avoid bubbling up changed notification when on new item/key of dictionaries to avoid inspector whipeout. Avoid bubbling up changed notification when on new item/key of dictionaries to avoid inspector wipeout. Sep 5, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Sep 6, 2024
@AThousandShips AThousandShips requested a review from a team September 6, 2024 12:47
@fire fire requested a review from KoBeWi September 6, 2024 15:09
@fire
Copy link
Member

fire commented Sep 6, 2024

I don't know if this is correct. Needs someone to investigate.

@ajreckof ajreckof marked this pull request as draft September 6, 2024 16:02
@ajreckof
Copy link
Member Author

ajreckof commented Sep 6, 2024

I just remembered why I didn't do it while it was so simple to do. Doing it the easy way like I did makes the copy paste bug to reappear. I'll make a version that fixes both this and not make the copy paste bug reappear during this week-end. Shouldn't too hard but requires a bit more thought then this first attempt.

@ajreckof
Copy link
Member Author

ajreckof commented Sep 6, 2024

In the end I had some time rn so made the PR and here are some small hindsight on the subject.
So why the bug this fixes happens.
What happens is that modifying the libraries modifies the available animation available. The available animation are needed in the property current_animation as it is a drop down menu of all animation available. Hence whenever the list of animations are changed you need to update the list of properties. Unfortunately rn this means a full whipe out of the inspector.
You might have realised there is two possibilities other then what I propose here.
First we could finally enable the possibility to only update/remove/add properties in the list so that small modifications like that do not whipe-out the full inspector. This is something that is also needed to fix other bugs and as been discarded for now as it would be something that needs a lot of work. But since bugs that would highly benefit from it tends to stack, it might indicate that the work might be worth it.
The second possible fix is to only update the list of properties only if the list of animations is actually modified. In fact here when we type a string this does not affect the list of animation in any way hence there is no reason to whipe-out the

How the bug can happen outside of this scope
In fact everything that is done here could be replicated in GDScript (I could give a MRP if needed). The question that I'm wondering is wether this practice would be considered an user error as it is the user that is whiping out the inspector at every change even if there isn't the need to (in this situation we should modify our code as proposed in previous paragraphs) or it the fault of the engine that is reporting modification where there isn't (then what is needed is the change in this PR). Maybe both are valid and both fixes are needed.

@ajreckof ajreckof marked this pull request as ready for review September 6, 2024 16:33
@ajreckof ajreckof force-pushed the avoid-bubbling-up-changed-notification-when-on-new-item/key-of-dictionaries-to-avoid-inspector-whipeout- branch from 5fcff71 to 72f4394 Compare September 6, 2024 16:33
@Repiteo Repiteo merged commit 7d18547 into godotengine:master Jan 27, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 27, 2025

Thanks!

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.

AnimationPlayer dictionary for AnimationLibraries does not accept values after the first
5 participants