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

Fix crash during import bone remap. #87905

Closed
wants to merge 1 commit into from

Conversation

tdaven
Copy link
Contributor

@tdaven tdaven commented Feb 3, 2024

The HashMap being used by can fail to find a bone to remap which will result in a crash since the HashMap is const.

Use the find method on the hashmap to loop values and gracefully handle the failure to find the bone instead of crashing.

Fixes: #87904

@AThousandShips
Copy link
Member

While it shouldn't crash the fact that it didn't already use a permissive check makes me think this should cause an error instead and abort importing

@tdaven
Copy link
Contributor Author

tdaven commented Feb 3, 2024

I suspect that would change previous behavior where it would rename/remap the bones it couldn't match and not the others which might break peoples import workflows.

@AThousandShips
Copy link
Member

No previous behaviour would change though would it? It'd just crash before, I mean specifically when the original crash condition occurs?

@tdaven
Copy link
Contributor Author

tdaven commented Feb 3, 2024

Prior to 96a95cb this worked just fine and didn't crash either.

@AThousandShips
Copy link
Member

Wasn't aware of that part, but that was a case where it shouldn't have manipulated the map anyway so no intentional behaviour was changed

The map is intentionally filled with valid entries and then previously copied, so the values weren't supposed to be added I'd say

Id suspect the previous copy instead of passing as const was an oversight, which was exposed by that change

But I'm not sure, we'll see what the relevant teams say 🙂

@tdaven
Copy link
Contributor Author

tdaven commented Feb 3, 2024

I suspect we don't want to warn. I partially only added them to confirm it was working but hey are a bit too verbose. Maybe only in dev builds or if at all.

The HashMap being used by can fail to find a bone to remap which will result in a crash
since the HashMap is const.

Use the find method on the hashmap to loop values and gracefully handle the failure
to find the bone instead of crashing.
@tdaven
Copy link
Contributor Author

tdaven commented Feb 16, 2024

Adding MRP to help in getting this merged at some point.

bone-remap-mrp.zip

@fire
Copy link
Member

fire commented Feb 16, 2024

@TokageItLab if you want to review.

@tdaven
Copy link
Contributor Author

tdaven commented Feb 24, 2024

I don't believe t his PR is needed anymore. The merging of #81746 included the same type of changes(using the find methods on the hashmap).

@akien-mga akien-mga closed this Feb 25, 2024
@akien-mga
Copy link
Member

Thanks for the contribution!

@AThousandShips AThousandShips removed this from the 4.3 milestone Feb 25, 2024
@tdaven tdaven deleted the fix-bone-remap-crash branch December 11, 2024 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Crashing during bone remap
4 participants