Skip to content

Fix animation track subpath hint inference. #106580

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

Closed

Conversation

SaracenOne
Copy link
Member

@SaracenOne SaracenOne commented May 19, 2025

Attempts to fix the issue by removing the bezier hack and handling it in a more elegant way based on @YuriSizov's suggestion #102130 (comment)

I think this should be correct now, but another pair of eyes on this would be appreciated.

@SaracenOne SaracenOne marked this pull request as draft May 19, 2025 02:31
@SaracenOne SaracenOne force-pushed the animation_track_type_hint_fix branch from 0e1811a to 0b2a84b Compare May 19, 2025 02:32
@SaracenOne SaracenOne marked this pull request as ready for review May 19, 2025 02:34
@TokageItLab TokageItLab requested a review from a team May 19, 2025 04:49
@YuriSizov
Copy link
Contributor

YuriSizov commented May 19, 2025

Hey, thanks for working more on this! To be clear, I was talking specifically about this change (and the one before it that introduced Variant value = p_node->get(p_property);):

Which I think is still in effect as of this PR:

// Get the value from the subpath.
Variant value = p_node;
Vector<String> property_path = p_property.split(":");
for (const String &E : property_path) {
if (value.get_type() == Variant::OBJECT) {
Object *obj = value;
value = obj->get(E);
} else {
value = Variant();
break;
}
}

Though I didn't look too deep into the issues and might've misattributed the cause, I still think that piece needs to be changed. If for nothing else but to avoid inventing ad hoc solutions when there is already one in the engine 🙂

@akien-mga akien-mga added the cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release label May 19, 2025
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.

The issue still occurs when selecting a subpath when inserting a BezierTrack.

image
image

As mentions above, a more fundamental modification that does not utilize leftover_path may provide a more consistent solution.

@akien-mga
Copy link
Member

#99115 was marked as an immediate release blocker, which means we can't release beta 1 without fixing this (I'm not sure that's warranted given that the bug exists in 4.4 too, but I defer to the maintainers).

So what's the path to getting this finalized and merged this week?

@TokageItLab
Copy link
Member

I'll take a look at this over the weekend.

@SaracenOne
Copy link
Member Author

Superseded by #107241

@SaracenOne SaracenOne closed this Jun 9, 2025
@akien-mga akien-mga removed this from the 4.5 milestone Jun 9, 2025
@akien-mga akien-mga added archived and removed cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release labels Jun 9, 2025
@TokageItLab TokageItLab removed their assignment Jun 9, 2025
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.

AnimationTrackEditor sub-property handling is not correct
4 participants