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 issue #5461 (segfault after removing redundant materials) #5467

Merged
merged 12 commits into from
Apr 9, 2024

Conversation

stephengold
Copy link
Contributor

Update mNumMaterials before early return from RemoveRedundantMatsProcess::Execute() .

@stephengold
Copy link
Contributor Author

I tested this fix on the "Running.gltf" model from issue #5461 and verified that the command completes successfully: no segfault.
I believe this pull request solves #5461 (without introducing any new issues) and is ready to be integrated.

- Fix 2 possible memleaks.
@kimkulling
Copy link
Member

During the review I have detected 2 leaks.

@stephengold
Copy link
Contributor Author

@kimkulling Thanks for solving the memory leaks.

@kimkulling
Copy link
Member

Hm, tests are failing for g++ on Ubuntu. I cannot reproduce it on windows. I will try to analyze it with Linux.

@stephengold
Copy link
Contributor Author

I tried to reproduce the test failure on my Linux desktop and was unsuccessful: all 574 tests passed.
My desktop used GNU 11.4.0, just like the GHA runner.

@stephengold
Copy link
Contributor Author

It seems my merge solved the test failure!
I believe this PR is now ready for integration.

@stephengold
Copy link
Contributor Author

After another merge from 'master', the tests are failing again.

@stephengold
Copy link
Contributor Author

Once again, merging has solved the failing checks. Please integrate this PR.

@stephengold
Copy link
Contributor Author

After another merge from 'master', another failing check.
I see no alternative except to wait for another merge opportunity.

@stephengold
Copy link
Contributor Author

Once again, merging has solved the failing checks. @kimkulling please integrate this PR.

@kimkulling
Copy link
Member

Updated again, let's hope that the tests are green.

@kimkulling kimkulling merged commit 08c1adc into assimp:master Apr 9, 2024
8 checks passed
@kimkulling
Copy link
Member

Merged, thanks a lot for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants