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

Fixed AnimationTrackEditor's TransformTrack can't edit #48515

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented May 6, 2021

2021-05-07.5.13.14-1.mov

AnimationTrackEditor's TransformTrack was failing to get/set value because the type of key when getting dictionary's value was wrong in 4.0. This PR fixed it.

However, TransformTrack is working correctly in 3.2, so it is possible that there is a fundamental casting problem going on somewhere in 4.0.

@akien-mga
Copy link
Member

AnimationTrackEditor's TransformTrack was failing to get/set value because the type of key when getting dictionary's value was wrong in 4.0. This PR fixed it.

However, TransformTrack is working correctly in 3.2, so it is possible that there is a fundamental casting problem going on somewhere in 4.0.

Your fix basically changes the argument from StringName to String. There was a number of changes to the core around StringNames are possibly that creates an issue here. But I don't know if the proper fix is to pass a String as key, or fix whatever makes it fail with StringNames when it used to work.

Maybe the problem is in Dictionary.has()?

@TokageItLab
Copy link
Member Author

TokageItLab commented May 7, 2021

@akien-mga StringName in the Variant, StringName was cast to String in 3.2, but in 4.0, StringName is still kept StringName. This makes it appear that the Dictionary operator [] doesn't work as desired, so it may be better to cast StringName to String in the Dictionary operator [].

Variant.cpp
3.2

Variant::Variant(const StringName &p_string) {

	type = STRING;
	memnew_placement(_data._mem, String(p_string.operator String()));
}
Variant::operator StringName() const {

	if (type == NODE_PATH) {
		return reinterpret_cast<const NodePath *>(_data._mem)->get_sname();
	}
	return StringName(operator String());
}

4.0

Variant::Variant(const StringName &p_string) {
	type = STRING_NAME;
	memnew_placement(_data._mem, StringName(p_string));
}
Variant::operator StringName() const {
	if (type == STRING_NAME) {
		return *reinterpret_cast<const StringName *>(_data._mem);
	} else if (type == STRING) {
		return *reinterpret_cast<const String *>(_data._mem);
	}

	return StringName();
}

@TokageItLab
Copy link
Member Author

Close this and move to #48523

@TokageItLab
Copy link
Member Author

#43371, #44310, #48139, #48523
As with some PRs, it is still undecided where the cast from StringName to String should be done in c++. If we don't want internal casting, this PR is expected to make the appropriate changes, so I reopen it.

@TokageItLab TokageItLab force-pushed the fix-animation-transform-track-cannot-edit branch from cef34be to 00ac6ff Compare July 28, 2021 23:19
@TokageItLab
Copy link
Member Author

Fixed by #50057.

@TokageItLab TokageItLab closed this Aug 6, 2021
@TokageItLab TokageItLab deleted the fix-animation-transform-track-cannot-edit branch May 23, 2022 15:20
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.

3 participants