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 crash when a Collada file has an empty normal vector #280

Merged
merged 3 commits into from
Dec 16, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Dec 14, 2021

Signed-off-by: ahcorde [email protected]

🦟 Bug fix

Summary

If a collada file has defined normals but these are empty, then Ignition will crash. This should fix this crash

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@ahcorde ahcorde self-assigned this Dec 14, 2021
@ahcorde ahcorde requested a review from mjcarroll as a code owner December 14, 2021 08:54
@github-actions github-actions bot added 🌱 garden Ignition Garden 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress labels Dec 14, 2021
@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Merging #280 (05e6005) into ign-common4 (98b070d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-common4     #280   +/-   ##
============================================
  Coverage        77.01%   77.02%           
============================================
  Files               76       76           
  Lines            10725    10727    +2     
============================================
+ Hits              8260     8262    +2     
  Misses            2465     2465           
Impacted Files Coverage Δ
graphics/src/ColladaLoader.cc 85.20% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98b070d...05e6005. Read the comment docs.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

That looks good to me. I'm wondering if we should also check that inputRemappedNormalIndex is in norms on this line below, to be safe.

subMesh->AddNormal(norms[inputRemappedNormalIndex]);

@ahcorde ahcorde force-pushed the ahcorde/collada/fix_empty_normal branch from 7d27e30 to 0f40797 Compare December 15, 2021 09:29
Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde merged commit 0012ca5 into ign-common4 Dec 16, 2021
@ahcorde ahcorde deleted the ahcorde/collada/fix_empty_normal branch December 16, 2021 15:49
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-24-citadel-edifice-fortress/1241/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants