Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

File format loader fixes #237

Merged
merged 9 commits into from Sep 29, 2023
Merged

File format loader fixes #237

merged 9 commits into from Sep 29, 2023

Conversation

sfan5
Copy link
Member

@sfan5 sfan5 commented Sep 18, 2023

I gave up on the .x loader since there's too many bugs (there are remaining ones).

Although I tried to be careful testing is welcome to check that I didn't accidentally break any valid models.

Copy link
Contributor

@numberZero numberZero left a comment

Choose a reason for hiding this comment

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

Nice to see security enhancements)

Although I tried to be careful testing is welcome to check that I didn't accidentally break any valid models.

Isn’t that what unit tests are for?

source/Irrlicht/CImageLoaderBMP.cpp Outdated Show resolved Hide resolved
source/Irrlicht/CImageLoaderBMP.cpp Outdated Show resolved Hide resolved
source/Irrlicht/CImageLoaderBMP.cpp Outdated Show resolved Hide resolved
source/Irrlicht/CImageLoaderBMP.cpp Show resolved Hide resolved
source/Irrlicht/CXMeshFileLoader.cpp Show resolved Hide resolved
@sfan5
Copy link
Member Author

sfan5 commented Sep 22, 2023

Isn’t that what unit tests are for?

True.
I'm not very motivated to add them to this code because we hopefully have to never touch it again when GLTF takes over all modelling needs inside MT.

Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

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

It would be good to value-init headers before reading them, e.g. in the BMP loader, for the case read() doesn't read a full header.

We should at some point replace some of these loaders with libraries, e.g. sdl2_image for images.

Haven't tested. Unittests would be good.

source/Irrlicht/SB3DStructs.h Outdated Show resolved Hide resolved
source/Irrlicht/CXMeshFileLoader.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 merged commit d767d27 into master Sep 29, 2023
20 of 28 checks passed
@sfan5 sfan5 deleted the secfix branch September 29, 2023 11:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants