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

Implement record feature for AnimationTree. #77395

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Daylily-Zeleen
Copy link
Contributor

@Daylily-Zeleen Daylily-Zeleen commented May 23, 2023

Impelent record featrure to instead of #77347.

AnimRecord.2.mp4

Bugsquad edited: Closes godotengine/godot-proposals#6807

Demo project:

animation_rollback_test.zip

Simple benchmark between List<Record>, LocalVector<Record> and CircularDeque<Record> for this use case:

benchmark

@Daylily-Zeleen Daylily-Zeleen requested review from a team as code owners May 23, 2023 13:14
@Calinou Calinou added this to the 4.x milestone May 23, 2023
scene/animation/animation_tree.h Outdated Show resolved Hide resolved
scene/animation/animation_tree.h Outdated Show resolved Hide resolved
scene/animation/animation_tree.h Outdated Show resolved Hide resolved
scene/animation/animation_tree.h Outdated Show resolved Hide resolved
@jcostello
Copy link
Contributor

How does it work?

@Daylily-Zeleen Daylily-Zeleen marked this pull request as draft May 24, 2023 02:56
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/animation_tree_record branch 3 times, most recently from bbb777e to c94c245 Compare May 25, 2023 12:48
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/animation_tree_record branch from c94c245 to f041a38 Compare May 26, 2023 05:50
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/animation_tree_record branch from f041a38 to 5c9cc26 Compare May 26, 2023 09:25
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/animation_tree_record branch 4 times, most recently from 5cbbce0 to bef82cf Compare May 26, 2023 11:02
@Daylily-Zeleen Daylily-Zeleen marked this pull request as ready for review May 26, 2023 11:03
@Daylily-Zeleen Daylily-Zeleen requested a review from a team as a code owner May 26, 2023 11:03
scene/animation/animation_tree.h Outdated Show resolved Hide resolved
Comment on lines 2243 to 2246
new_record.state_last_pass = state.last_pass;
new_record.state_valid = state.valid;
new_record.process_pass = process_pass;
Copy link
Member

@TokageItLab TokageItLab May 27, 2023

Choose a reason for hiding this comment

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

I think these three variables are not necessary. Instead, I guess you may need some way to avoid doing double processing after the call to load_record().

Copy link
Contributor Author

@Daylily-Zeleen Daylily-Zeleen May 27, 2023

Choose a reason for hiding this comment

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

state.last_pass will be overrided in _process_graph, it can't be removed.

It seem state.last_pass and process_pass only be used to get activity in editor plugin,
state.valid will be overrided in _process_graph, too, and it is not be expoed to GDScript, It seem only work with state.state_invalid_reasons in editor.

In conclusion, I remove state.last_pass, and handel process_pass, state.valid and state.state_invalid_reasons only in tool build.

I guess you may need some way to avoid doing double processing after the call to load_record().

I am not sure about what do you mean of avoiding double processing.
load_record() just retrieve the state, it not do any processing of animation, it means that skeleton pose or any other animation datas is not be saved as record.
May be we should add signals to notify users that there have a new record be saved (for auto recording).

Copy link
Member

@TokageItLab TokageItLab May 27, 2023

Choose a reason for hiding this comment

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

I wanted to say that instead of recording these, it is necessary to set it up again after retrieving the parameter list with load_record().

Copy link
Contributor Author

@Daylily-Zeleen Daylily-Zeleen May 27, 2023

Choose a reason for hiding this comment

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

Sorry, I still not clearify what do you mean.
I have changed the retrieving order of Record's members in load_record(). Please review it.

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/animation_tree_record branch 3 times, most recently from a1a6e51 to d152e5b Compare May 27, 2023 08:41
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/animation_tree_record branch 3 times, most recently from 2a0b9d8 to d1d1a70 Compare May 27, 2023 20:12
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.

Expose low-level properties of AnimationTree
6 participants