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 editor crash when re-importing GLTF while animation is playing #83104

Conversation

jsjtxietian
Copy link
Contributor

@jsjtxietian jsjtxietian commented Oct 10, 2023

Fixes #82962

This might not be a good solution as it stops the animation everytime edit is called.
Do we have a way to distinguish between normal edit and edit caused by resource re-import? I would appreciate any feedback or alternative suggestions.

@jsjtxietian jsjtxietian marked this pull request as ready for review October 10, 2023 12:59
@akien-mga akien-mga requested review from a team October 10, 2023 13:18
@akien-mga akien-mga added this to the 4.2 milestone Oct 10, 2023
Copy link
Member

@TokageItLab TokageItLab 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 this is not good way because it will affect even the cases that have nothing related to the reimport.

I suggest that get the singleton of PlayerEditor in importer or somewhere and call the function which stop the player only when a reimport occurs. However, I wonder if stop() is really necessary; just regenerating the cache with clear_caches() may be sufficient. Well, it should not be done in edit() anyway.

@jsjtxietian
Copy link
Contributor Author

I suggest that get the singleton of PlayerEditor in importer or somewhere and call the function which stop the player only when a reimport occurs. However, I wonder if stop() is really necessary; just regenerating the cache with clear_caches() may be sufficient. Well, it should not be done in edit() anyway.

Thanks! I did not realize there is a singleton of PlayerEditor!

@jsjtxietian jsjtxietian force-pushed the fix-crash-when-reimport-animation-while-playing branch from a80c63f to 3cf3044 Compare October 11, 2023 04:58
@jsjtxietian
Copy link
Contributor Author

However, I wonder if stop() is really necessary; just regenerating the cache with clear_caches() may be sufficient.

IMHO it's fine to stop it, as let the animation playing while reimporting is a very rare case and it's not unintuitive to see the animation stopped when re-imported.

editor/editor_node.cpp Outdated Show resolved Hide resolved
@jsjtxietian jsjtxietian force-pushed the fix-crash-when-reimport-animation-while-playing branch from 3cf3044 to cd4a52e Compare October 12, 2023 04:42
editor/editor_node.cpp Outdated Show resolved Hide resolved
@jsjtxietian jsjtxietian force-pushed the fix-crash-when-reimport-animation-while-playing branch 2 times, most recently from 640613c to 8bd3a3b Compare October 13, 2023 10:12
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

Considering multithreading, it may be fine to stop before re-importing as just you have written.

Ideally, it would be better to resume playback if the animation has not changed after re-import, but given that this is editor-only, it is probably not worth it.

editor/editor_node.cpp Outdated Show resolved Hide resolved
@jsjtxietian jsjtxietian force-pushed the fix-crash-when-reimport-animation-while-playing branch from 8bd3a3b to 3c10493 Compare November 15, 2023 03:13
@akien-mga akien-mga merged commit 6d47eff into godotengine:master Nov 15, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@jsjtxietian jsjtxietian deleted the fix-crash-when-reimport-animation-while-playing branch November 15, 2023 15:31
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.

Editor crashes when re-importing GLTF scene while animation is playing.
4 participants