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

Replace old stb_image 1.18 with stb_image 2.30 and stb_image_write 1.16 #53

Merged
merged 6 commits into from
Jun 28, 2024

Conversation

illwieckz
Copy link
Member

Replace old stb_image with stb_image 2.30 and stb_image_write 1.16.

Pulled from https://github.com/nothings/stb
Commit 013ac3beddff3dbffafd5177e7972067cd2b5083

Fixes #13:

…write 1.16

Pulled from https://github.com/nothings/stb
Commit 013ac3beddff3dbffafd5177e7972067cd2b5083

Fixes #13
…e_write 1.16

Pulled from https://github.com/nothings/stb
Commit 013ac3beddff3dbffafd5177e7972067cd2b5083
…e_write 1.16

Pulled from https://github.com/nothings/stb
Commit 013ac3beddff3dbffafd5177e7972067cd2b5083
crnlib/stb_image_write.h Fixed Show fixed Hide fixed
crnlib/stb_image_write.h Fixed Show fixed Hide fixed
crnlib/stb_image_write.h Fixed Show fixed Hide fixed
crnlib/stb_image_write.h Fixed Show fixed Hide fixed
crnlib/stb_image_write.h Fixed Show fixed Hide fixed
@illwieckz
Copy link
Member Author

illwieckz commented Jun 25, 2024

To know the modifications that was previously added to the old copy of stb_image, one can compare from the commit before this branch the example1/stb_image.h file with the crnlib/crn_image_utils.cpp file which was just a copy of the same stb_image.h at the time plus some minor changes.

The big difference is that previously, the stb functions were declared in namespace crnlib, it si not anymore.

Other changes is that many minor fixes implemented on crnlib/crn_image_utils.cpp are now lost, but all of them are probably useless and has been likely implemented by upstream in newer stb_image.h. In fact, I managed to produce both Linux, MinGW, macOS and FreeBSD builds without problems.

I guess it means this can introduce a mismatch if crnlib is linked with another software having built another version of stb.

@illwieckz
Copy link
Member Author

The update of stb_image will also help to make the examples less Windows-centric, as the very old stb_image was Windows-centric and the copy in crnlib received fixes before.

@illwieckz
Copy link
Member Author

So, Appveyor says it also builds on MSVC. So it builds everywhere as I verified myself the others.

crnlib/stb_image_write.h Fixed Show fixed Hide fixed
@illwieckz illwieckz changed the title Replace old stb_image with stb_image 2.30 and stb_image_write 1.16 Replace old stb_image 1.18 with stb_image 2.30 and stb_image_write 1.16 Jun 25, 2024
@illwieckz illwieckz force-pushed the illwieckz/stb-image branch 2 times, most recently from 1973ac0 to 13f0de3 Compare June 25, 2024 22:42
@DolceTriade
Copy link

Did you make any modifications to the upstream file?

@illwieckz
Copy link
Member Author

illwieckz commented Jun 27, 2024

Except the changes done in stb_image_write: prevent int to overflow before converting it to size_t commit (which are preventing some integers to overflow as reported by CodeQL), the stb files are an unmodified a copy-paste from stb upstream. I plan to submit the patch to stb upstream. My purpose is that future stb updates would just copy-paste the stb upstream.

Edit: I submitted the changes upstream:

@illwieckz
Copy link
Member Author

illwieckz commented Jun 27, 2024

This reintroduces a sprintf.

They have a pending PR for it:

Edit: I implemented a similar fix.

@illwieckz illwieckz force-pushed the illwieckz/stb-image branch 2 times, most recently from b4a5a37 to 19f931a Compare June 27, 2024 23:56
@@ -8,6 +8,8 @@ set(EXAMPLE2_SRCS
${CMAKE_CURRENT_SOURCE_DIR}/example2.cpp
${CMAKE_CURRENT_SOURCE_DIR}/timer.cpp
${CMAKE_CURRENT_SOURCE_DIR}/timer.h
${CMAKE_CURRENT_SOURCE_DIR}/../inc/crnlib.h
Copy link
Member

Choose a reason for hiding this comment

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

Headers of a dependent library don't really belong in the source list. Same goes for example3

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't it what CMake use to know the source has to be rebuilt if the header changes?

Copy link
Member

Choose a reason for hiding this comment

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

No. AFAIK the only use is to index it in IDE projects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, wouldn't it be a good idea though? I see there are some VS projects there but they are likely unmaintained and will be out of date once this is merged.

Copy link
Member

Choose a reason for hiding this comment

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

No. In the VS project setting, headers that are part of the Crunch library should be in the library's sources, not the example sources.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I removed them.

@illwieckz illwieckz merged commit 50a20aa into master Jun 28, 2024
3 checks passed
@illwieckz illwieckz deleted the illwieckz/stb-image branch June 28, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crunch is known to not properly grok all png format variants
3 participants