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

Add persistent folding to Animation Library Editor #86481

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

Illauriel
Copy link
Contributor

Enables folding of the library entries in Animation Library Editor, necessary on big projects with hundreds of animations to prevent excessive scrolling.

For folding to be persistent, the collapsed state of TreeItems is saved in a subfolder of .godot/editor/ or wherever get_project_settings_dir() would point. I'd love to do it via editor_folding.ccp, but I couldn't find out how to repurpose existing methods from that class, and didn't want to disrupt anything by accident. Because of that all file manipulations are performed in animation_library_editor.cpp.

Due to the fact that multiple AnimationMixers can be present in a scene and share same libraries, the persistence files are created per mixer node path and not per scene. Filename is formed as (pseudocode):
"folding-" + (scene_file_path + mixer_node_path).md5()+".cfg"
Because of this, clutter of outdated files can form when the scene filename or the mixers' nodepaths change.

I would be glad if someone more knowledgeable could suggest how to do persistence in editor more gracefully.^^'

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.

@TokageItLab I think adding persistent folding to the animation library editor is fine, but I also want your review.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 11, 2024

lib_fold should be a single file, not a whole folder. You can make it a ConfigFile and use sections for each AnimationMixer, instead of files.

@Illauriel
Copy link
Contributor Author

lib_fold should be a single file, not a whole folder. You can make it a ConfigFile and use sections for each AnimationMixer, instead of files.

Sounds reasonable. I was kinda mimicking how scene view folding has separate file for each scene, but it irked me that it creates clutter. I'll do an attempt to change it tonight :)

@Illauriel Illauriel force-pushed the add-animlib-folding branch 2 times, most recently from df040d8 to 18314e9 Compare February 12, 2024 16:50
@Illauriel
Copy link
Contributor Author

Illauriel commented Feb 12, 2024

Changed _load_lib_folding() and _update_lib_folding() to use a single lib_folding.cfg() as per KoBeWi's suggestion. Removed _create_fold_dir().
Hope it looks a little cleaner now ^^'

UPD: Removed a dangling variable left from the previous implementation.

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 agree with the implementation of folding, and using loosely coupling to implement it with the actual object in the scene is good from a maintainability standpoint.

On the other hand, there is a problem of unstable behavior when renaming, since the path for storing the folding state depends on the name.

Renaming the AnimationMixer in the SceneTree clears all the folding state. In addition, if the names of the two AnimationMixers are switched, the state of the AnimationLibrary with same name will be strange.

image

When renaming an AnimationLibrary in the folding state, it expands folding at the moment of renaming and collapses folding when renamed again to the original name.

image

I think it would be fine to store the state by path by name when saving, but it would be necessary to tie them together with ObjectID or pointer while the editor is running. In other words, it should have paths by ObjectID while the editor is running and needs to add methods that will convert the path between name and ObjectID when the scene is opened and when it is saved.

@Illauriel
Copy link
Contributor Author

Illauriel commented Feb 13, 2024

On the other hand, there is a problem of unstable behavior when renaming, since the path for storing the folding state depends on the name.

You're right! I never figured out how to do it correctly so that renames and changing Mixer's place in hierarchy doesn't affect it, but using ObjectID sounds promising! I'll try^^

it should have paths by ObjectID while the editor is running and needs to add methods that will convert the path between name and ObjectID when the scene is opened and when it is saved.

Current implementation saves state when it gets a signal from TreeItem in the AnimationLibraryEditor window, I'm not sure how to differentiate between "editor runtime" and "when the scene is being saved", but I'll probably try to store both name and ID and use what fits^^'

@TokageItLab
Copy link
Member

TokageItLab commented Feb 13, 2024

The ObjectID is updated by memory allocation and should not be stored and therefore needs to be converted with the name.

The conversion is probably needed when opening the scene and when saving, which can be detected in editor_node.cpp. Hardcoding into the EditorNode is not exactly good, but there are already a few animation-related things that are hardcoded there so it would be fine for now.

@Illauriel
Copy link
Contributor Author

Illauriel commented Feb 14, 2024

The ObjectID is updated by memory allocation and should not be stored and therefore needs to be converted with the name.

So over the night, I've managed to solve problem # 2 where renaming libraries was causing fold state to revert to unfolded.
I still need to solve # 1 where renaming the mixer itself causes loss of saved state, and to decide how to cleanup old names from the save file. Should be doable. ^^'

PS: So far I've managed to avoid messing with editor_node.cpp, and to be honest I don't want to touch an almost 8K line file with a 15 foot pole.

@Illauriel Illauriel requested a review from a team as a code owner November 14, 2024 23:43
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.

Looks good.

I contributed the signature idea to @Illauriel, and we discussed the idea at the animation meeting.

I still need to test the code.

@Illauriel
Copy link
Contributor Author

Illauriel commented Nov 15, 2024

Looks good.

I contributed the signature idea to @Illauriel, and we discussed the idea at the animation meeting.

I still need to test the code.

Thanks!

I actually found another edge case, but it's a quick thing to fix. Gonna make an edit in a few min.

Upd: Fixed!

@Illauriel
Copy link
Contributor Author

Okay, I think now we're good. Phew, it's been a while!

On the other hand, there is a problem of unstable behavior when renaming, since the path for storing the folding state depends on the name.

I addressed all cases where editing names of Mixers, names of Libraries, or moving Mixer in the hierarchy would lose folding state.

I think it would be fine to store the state by path by name when saving, but it would be necessary to tie them together with ObjectID or pointer while the editor is running.

I followed this suggestion, and started storing both names and ObjectIDs for current session. If ObjectIDs are outdated (Usually because it's a new session) it falls back on names again. In case the hierarchy has changed, and some other cases, it tries to recover data, looking for current mixer's ObjectID in other saved entries. Another fallback that @fire mentioned above is so called "mixer signature" - a hash of all the libraries and animations in the mixer. It should be sufficient to recover for all remaining cases, unless the library list is edited via code. But since in that case user doesn't interact with AnimationLibraryEditor at all, folding shouldn't matter all that much.
Also the system tries its best to cleanup and actualize outdated entries.

Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

Was discussed and demonstrated in the Animation meeting and seems fine 👍

@fire fire requested a review from Repiteo November 21, 2024 20:04
@Repiteo Repiteo merged commit 2993289 into godotengine:master Nov 22, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 22, 2024

Thanks!

@Illauriel Illauriel deleted the add-animlib-folding branch November 22, 2024 00:13
@AThousandShips AThousandShips modified the milestones: 4.x, 4.4 Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants