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

core:image add support for option .flip_vertical #4826

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

DerTee
Copy link

@DerTee DerTee commented Feb 11, 2025

This is basically the same as the option in stb_image to flip an image. One common use case is loading images for OpenGL which expects pixel data to be upside down, unlike most other graphics APIs and image formats.

I'm not super sure if this is useful to most people, plus it's trivial to write yourself if you do need it, so no problem if you decide not to accept this PR just for irrelevance :)

@gingerBill gingerBill requested a review from Kelimion February 11, 2025 22:40
@laytan
Copy link
Collaborator

laytan commented Mar 13, 2025

My opinion on this, (but would love a review from @Kelimion) is that the helper function is a nice addition but having it as an option on top isn't needed.

@@ -146,6 +148,7 @@ Option :: enum {
alpha_drop_if_present, // Unimplemented for QOI. Returns error.
alpha_premultiply, // Unimplemented for QOI. Returns error.
blend_background, // Ignored for non-PNG formats
vertical_flip, // flip image vertically on load
Copy link
Member

Choose a reason for hiding this comment

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

For consistency with the other comments, I'd capitalize // Flip.

@@ -1438,6 +1441,17 @@ expand_grayscale :: proc(img: ^Image, allocator := context.allocator) -> (ok: bo
return true
}

vertical_flip :: proc(img: ^Image) {
pixels := img.pixels.buf[:]
bpp := img.depth/8 * img.channels
Copy link
Member

Choose a reason for hiding this comment

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

This proc should probably have a comment that it only supports 8 and 16 bpp, and an assert at the top that enforces this.

@Kelimion
Copy link
Member

For completeness, it would be great if this new option had some testing as well. Maybe a handful of images of 8 and 16 bpp that we load twice, once with .flip_vertical and once plain, which are then compared.

DerTee added 4 commits March 14, 2025 04:19
This is basically the same as the option in stb_image to flip an image.
One common use case is loading images for OpenGL which expects pixel data to be
upside down, unlike most other graphics APIs and image formats.
small cleanups:
 - doc comments for proc and option
 - make whitespace consistent
 - make comment capitalization consistent
@DerTee DerTee force-pushed the image_flip_vertically branch from d40e436 to be6ff42 Compare March 14, 2025 04:55
@DerTee
Copy link
Author

DerTee commented Mar 14, 2025

@laytan

My opinion on this, (but would love a review from Kelimion) is that the helper function is a nice addition but having it as an option on top isn't needed.

I agree in principle. Still there are reasons, both very minor, admittedly, to include the option:

  • BMPs can be encoded in a flipped format (negative height, iirc). There is already code in the BMP loader that flips the image. By making the flip an option, we can detect that during load and avoid unnecessarily flipping the image twice. Not a big deal of course, but it's slightly nicer.
  • People use stb_image a lot and it has a vertical_flip option, plus it might add to discoverability.

@DerTee
Copy link
Author

DerTee commented Mar 14, 2025

@Kelimion
I still need to write the tests, no need to review yet.

@Kelimion
Copy link
Member

Kelimion commented Mar 14, 2025

  • BMPs can be encoded in a flipped format (negative height, iirc). There is already code in the BMP loader that flips the image. By making the flip an option, we can detect that during load and avoid unnecessarily flipping the image twice. Not a big deal of course, but it's slightly nicer.

TGA as well. See the variable origin_is_top, set if the image height is negative, which then starts at the bottom and increases the line offset by -1 instead of +1. You're currently flipping the image using the helper, but that variable could also take the option into account to not do that work twice.

@laytan
Copy link
Collaborator

laytan commented Mar 14, 2025

Alright, I am fine with the option then

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.

None yet

3 participants