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.

Sign up for GitHub

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

Jump to bottom

Reimport bone attachment fixes #82471

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Reimport bone attachment fixes #82471

merged 1 commit into from
Oct 30, 2023

Conversation

SaracenOne
Copy link
Member

@SaracenOne SaracenOne commented Sep 28, 2023 •
edited by AThousandShips
Loading

Fixes an oversight of accidentally giving NOTIFICATION_NODE_RECACHE_REQUESTED the same ID as NOTIFICATION_DRAW, and adds the notification to BoneAttachment3D and Skeleton3D

Fixes some bugs in GLTF importing which would lead to issues when reimporting the scene. An earlier version of this PR also made use of the NOTIFICATION_NODE_RECACHE_REQUESTED notification, but discussions in RocketChat indicated that usage of this notification may not have been desirable, so for now, I've included a bespoke recursive function with special-case handling for Skeleton3Ds and BoneAttachment3Ds and removed dependency on NOTIFICATION_NODE_RECACHE_REQUESTED.

Closes #82466
Closes #82680

@SaracenOne SaracenOne added this to the 4.2 milestone Sep 28, 2023
@SaracenOne SaracenOne self-assigned this Sep 28, 2023
@SaracenOne SaracenOne force-pushed the recache_bone_attachment branch 2 times, most recently from ca4848c to b2224c9 Compare September 28, 2023 07:42
@SaracenOne SaracenOne marked this pull request as ready for review September 28, 2023 07:43
@SaracenOne SaracenOne requested review from a team as code owners September 28, 2023 07:43
@fire fire requested a review from a team September 28, 2023 07:45
@SaracenOne SaracenOne force-pushed the recache_bone_attachment branch from b2224c9 to cfbafc6 Compare September 28, 2023 07:45
@SaracenOne SaracenOne marked this pull request as draft September 28, 2023 09:09
@SaracenOne SaracenOne force-pushed the recache_bone_attachment branch from cfbafc6 to 8ef1cd5 Compare September 28, 2023 11:45
@SaracenOne
Copy link
Member Author

SaracenOne commented Sep 28, 2023 •
edited
Loading

I've updated this fix a bit more throughly to also have the GLTF importer assign the correct bone_idx at this stage. This needs to be done because otherwise, it doesn't get saved and we cannot derive the correct reset value from any scene which derive from it. This silences some warnings on reimport. I've also forced the bone_idx to -1 during the RECACHE notification to ensure that the idx will get properly updated.

An issue remaining is that with changing the enum values, the build system warns of compat breakage with GDExtension. Since this is an enum which I imagine nobody used it was only designed to deal with bugs like this, would request information on how to proceed.

@SaracenOne SaracenOne marked this pull request as ready for review September 28, 2023 11:48
@SaracenOne SaracenOne requested a review from a team as a code owner September 28, 2023 11:49
@Sauermann
Copy link
Contributor

part of #75839

@slumberface
Copy link

slumberface commented Sep 29, 2023 •
edited
Loading

Tested this with a build using 8ef1cd5. The main issue in ticket #82466 seems to be fixed! The armature itself no longer appears to glitch visually. But by adding a total of 6 bone attachments, and then removing 2, the lowest bone attachment in the hierarchy stays at idx 16 (incorrectly pointing to a idx associated with the armature), instead of the new correct idx which appears to be 14.
Incorrect:
image
Correct after manually adjusting:
image

@SaracenOne
Copy link
Member Author

Hmm, would it be possible to do another reproduction project for this specific instance too @slumberface? I just want to make sure I'm not making a mistake and am actually reproducing the situation as accurately as possible.

@slumberface
Copy link

@SaracenOne I can definitely make another reproduction that demonstrates this last situation. Do you want it made on 4.1.1 or 8ef1cd5?

@SaracenOne
Copy link
Member Author

Thanks! @slumberface
8ef1cd5 Would probably be easier for me

@slumberface
Copy link

@SaracenOne I'm experiencing an issue where the Godot editor/engine itself won't compress in windows (file names that are too long??). Here is the asset folder you can drag into a 4.1.1 project. Same instructions for reproduction but this test has:

  • 6 BoneAttachments to begin with
  • Instead of adding BoneAttachments, 2 are removed when Model_A is overwritten with Model_B (the first test was only adding bone attachments and I believe your branch seems to fix that scenario)

Assets for BoneAttachment Bug.zip

@fire fire mentioned this pull request Oct 4, 2023
Open
@fire fire mentioned this pull request Oct 12, 2023
Open
@SaracenOne SaracenOne force-pushed the recache_bone_attachment branch from 8ef1cd5 to f920bf3 Compare October 19, 2023 13:24
@SaracenOne
Copy link
Member Author

@slumberface Sorry for the delay in updating this. Thanks a lot for your updated MRP. I think it should be properly fix now; the issue appears to be the fact that while I added the correct IDX assignment for one part of the GLTF importer, there was another branch where it needed to be added too.

With that in mind, I think the only thing remaining on this PR is knowing how to go about dealing with the BONE_RECACHE enum issue I've mentioned.

@slumberface
Copy link

@SaracenOne no worries on the delay and good work circling back to this!

@AThousandShips
Copy link
Member

This notification collision also causes this:

@AThousandShips
Copy link
Member

Do we want to just remove calling the notification for 4.2 and do an overhaul in 4.3, we have issues arising from it and close to release

@SaracenOne SaracenOne force-pushed the recache_bone_attachment branch from f920bf3 to 904a552 Compare October 27, 2023 18:51
@SaracenOne
Copy link
Member Author

SaracenOne commented Oct 27, 2023 •
edited
Loading

PR's been updated with a more ad-hoc fix for this bug and removed dependency on the notificiation which was discussed to potentially be undesirable. We can look into how to more universally handle notifying nodes of scene changes for 4.3 since removing the notification means that editors scripts have no awareness of when a reimport has occured.

@SaracenOne SaracenOne changed the title NOTIFICATION_NODE_RECACHE_REQUESTED fixes Reimport bone attachment fixes Oct 27, 2023
YuriSizov
YuriSizov reviewed Oct 27, 2023
View reviewed changes
scene/3d/skeleton_3d.cpp Outdated Show resolved Hide resolved
YuriSizov
YuriSizov approved these changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Approving for the overall consensus and for the editor side of changes. Should be okay for now, but we will need to rework it later, of course.

Needs some clean up, and an approval from the animation team would be appreciated.

@SaracenOne
Reimport bone attachment fixes:
4b671ee 
Assign bone_idx to GLTF importer to fix serialization.
Notifies Skeletons and BoneAttachments when reimporting.
Removes usage of NOTIFICATION_NODE_RECACHE_REQUESTED
@SaracenOne SaracenOne force-pushed the recache_bone_attachment branch from 904a552 to 4b671ee Compare October 27, 2023 19:34
AThousandShips
AThousandShips approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM

@AThousandShips AThousandShips requested a review from reduz October 27, 2023 19:40
akien-mga
akien-mga approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@akien-mga akien-mga merged commit e2c79bc into godotengine:master Oct 30, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov mentioned this pull request Nov 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips approved these changes

@YuriSizov YuriSizov YuriSizov approved these changes

@akien-mga akien-mga akien-mga approved these changes

@reduz reduz Awaiting requested review from reduz

Assignees

@SaracenOne SaracenOne

Labels
bug topic:animation topic:editor
Projects
None yet
Milestone
4.2
6 participants
@SaracenOne @Sauermann @slumberface @AThousandShips @akien-mga @YuriSizov