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

Remove support for legacy bone binds from godot. All skin binds must be named #48914

Closed
wants to merge 1 commit into from

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented May 21, 2021

@fire informed me that numbered bone binds for Skin are a legacy feature, and that godot is moving in the direction of using named binds for Skins.

After running into the issue leading to PR #48913 , I think that it is error-prone and confusing to support two different Skin systems. Clearly this change should not be made in the 3.x branch as it could break compatibility, but I think 4.x ought to consider removing the legacy system.

@akien-mga akien-mga added this to the 4.0 milestone May 21, 2021
@akien-mga akien-mga requested review from a team and reduz May 21, 2021 07:33
@lyuma lyuma force-pushed the remove_legacy_bone_binds branch 2 times, most recently from cb5d948 to 6b60dd0 Compare May 21, 2021 14:16
@lyuma lyuma marked this pull request as ready for review May 21, 2021 14:17
@lyuma lyuma requested review from a team as code owners May 21, 2021 14:17
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.

I approve of this, but I think the decision is on reduz. He's the expert on this.

We chatted in Godot Engine chat about how backwards compatibility breakage is not a concern, but it wasn't clear why this is blocked.

@reduz
Copy link
Member

reduz commented Jun 11, 2021

Scenes made in 3.x wont work, although it may be an option to ask users to re-import them with this setting on before loading them on 4.0. Not sure what others think about it.

@fire
Copy link
Member

fire commented Jun 11, 2021

We discussed bind id (integer) vs bind name (string) between I and Lyuma:

  1. Most of the time, users will be using skins imported from a glb that is already using bind names.
  2. A revision to this proposal is to force the remaining importers to use bind names in 3.x and then remove bind ids in 4.x.
  3. This implies a workflow that people need to open the project in compatibility upgrade version of 3.x.

@fire
Copy link
Member

fire commented Jul 30, 2021

@lyuma Task to rebase.

@lyuma lyuma force-pushed the remove_legacy_bone_binds branch from 6b60dd0 to 521f2fb Compare August 21, 2021 01:08
@lyuma
Copy link
Contributor Author

lyuma commented Aug 22, 2021

Just to note, this change will break all existing Skin objects and require a reimport. I tried opening a project from an older build of godot 4, and I get these errors:

ERROR: Skin bind #0 does not contain a name nor a bone index.
   at: Skeleton3D::_notification (scene\3d\skeleton_3d.cpp:249)
ERROR: Skin bind #1 does not contain a name nor a bone index.
   at: Skeleton3D::_notification (scene\3d\skeleton_3d.cpp:249)
ERROR: Skin bind #2 does not contain a name nor a bone index.
   at: Skeleton3D::_notification (scene\3d\skeleton_3d.cpp:249)

It looks like this:
image

(all bones appear to be bound to bone 0)

@lyuma
Copy link
Contributor Author

lyuma commented Nov 30, 2021

Closing because there wasn't consensus.

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.

4 participants