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 an import setting use_legacy_names for 3.3 compatibility. #48058

Merged
merged 1 commit into from
Apr 21, 2021

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Apr 21, 2021

During the development of 3.3, internationalization features were added to allow arbitrary bone and node names.
However, doing so will break all references and existing animation clips for projects upgraded from 3.2
This adds an import setting, enabled by default, but disabled for newly generated .import files which restores the old behavior.

This addresses issue #47977.

PR #47742 was recently merged, but is insufficient to cover the whole issue. I made that PR part of the use_legacy_names import setting introduced in this change.

This commit effectively reverts the three commits f1e8ec9 and b032067 and 1f87bca behind the use_legacy_names import setting.

CC @fire @akien-mga

image

A new fairly comprehensive test project generated from 3.2.3 is here:
BoneSanitizationFixed.zip

based on the following model:
https://github.com/godotengine/godot-tests/blob/master/tests/blend_export/gltf/45545-relax-name-sanitization.gltf
Includes scene node references, cross-scene animation references and other fun cases.

Old version of test project is here: BoneSanitization.zip Apparently some changes were made in October so this project has bone names with "." in them which was not allowed in 3.2.3

@lyuma lyuma requested a review from a team as a code owner April 21, 2021 04:12
@lyuma lyuma marked this pull request as draft April 21, 2021 04:37
During the development of 3.3, internationalization features were added to allow arbitrary bone and node names.
However, doing so will break all references and existing animation clips for projects upgraded from 3.2
This adds an import setting, enabled by default, but disabled for newly generated .import files which restores the old behavior.
@lyuma lyuma force-pushed the legacy_names_gltf_3.3 branch from 52f436a to d92a172 Compare April 21, 2021 05:49
@lyuma lyuma marked this pull request as ready for review April 21, 2021 05:53
@lyuma
Copy link
Contributor Author

lyuma commented Apr 21, 2021

Note that this change does not affect DAE or OBJ formats.

I am pretty sure that the only place those files can be affected by this change is the call to node->set_name() as that is where the excess validation was formerly done:

editor/import/resource_impoter_obj.cpp line 440:

mi->set_name(E->get()->get_name());

and editor/import/editor_import_collada.cpp line 333:

node->set_name(p_node->name);

Sanitization might be needed there if compatibility is desired for obj scenes or collada.
It's hard for me to evaluate if it's good or bad to make these changes, and I don't have any good test models to use.

Comment on lines +260 to +262
if (name.empty()) {
name = "bone";
}
Copy link
Member

@akien-mga akien-mga Apr 21, 2021

Choose a reason for hiding this comment

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

This part is missing from the legacy branch. Though I guess this might be on purpose, to keep the same behavior as before (i.e. potentially an empty bone name?).

@akien-mga akien-mga merged commit d91b780 into godotengine:3.x Apr 21, 2021
@akien-mga
Copy link
Member

Thanks!

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.

2 participants