-
-
Notifications
You must be signed in to change notification settings - Fork 20.2k
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
Adds 16bpp support for BMP File Format #67608
Adds 16bpp support for BMP File Format #67608
Conversation
I also added the test project I was using while testing the code here: https://github.com/ScorpionInc/godot-projects/tree/main/BMPTest if that would help anyone reviewing out. It's not the prettiest project but it uses alot of different BMP image formats and colors so you can see the edge cases better. |
The check: "Static Checks / Static Checks (clang-format, black format, file format, documentation checks)" Fails still however the suggested results look the same as my current values. Is there some invisible format character in there somewhere that I'm missing? Thanks for your time. |
You need to use tabs for indentation, not spaces. Make sure you don't have mixed tab/space indentation too (we don't use spaces for alignment). From a quick look at the CI log, you appear to have mixed indentation on the lines highlighted in red. |
7bc32aa
to
48c748d
Compare
Alrighty should be ready now hopefully thanks as always for everyone's time and help. o/ |
You can check out with my MR #67802 too. |
If I remember correctly, Reduz didn't like color palletizing (color reduction) or using formats typically LDR for HDR because it caused confusion and thus made Godot Engine less usable. This appears to be converting RGB565, and RGB555 to RGBA8. So I think it's fine. |
I am not seeing where the color scaling is done. This is probably just something I missed because I'm dumb. I've tried compiling the code from your fork and testing it with many different 16 bit formats of BMP and have not gotten resuts I expected. This may be because I wrote my code and project for an older version of Godot and havent forwarded my code yet. However your code seems to produce the same results as stable branch but with more warnings and a black box vs more errors and transparent sprites. I'll try applying my code changes to the current version of Godot (4.1.dev.custom_build[21d080e] at this time) and see if I get simular results, but wanted to share what I'm seeing so far. Project: I'll keep you posted on what I find. I like the look up for masks, based on what I read in your merge it sounds like the missing piece to get some of the alpha formats working. I'm not tring to say anything bad about your code, just wanted to show I'm not seeing it working on my machine yet. I'll keep you guys posted on if updating the base code breaks my version as well. Have a great day o/. |
48c748d
to
ef02571
Compare
Sorry accidentally closed this while fetching upstream due to a file conflict(This has been fixed). |
Thought I'd share a little update. So far I got almost all versions of 16 bit bmps working im only having issues with the X8R8G8B8 format. for some reason the red mask reads a 0x0000 instead of the expected: 0x00FF. Interestingly A8R8G8B8 also does this however it displays correctly where X8 does not. Still working on it but I have masks implemented mostly and thought I'd share. Edit: So the reason I'm going to give up on the X8R8G8B8 format is for one it doesnt fit in the theme of this branch as it's a 32bit format and two im stumped. It's still something I'd like to fix eventually but I'll try it in a seperate branch. Also I found that a combination of lacking color scaling and having moved the code into the switch is what caused the black textures I got when using Anutrix's code. Leaving that part alone and 16BPP works fine. I'll be updating on github soon(Hopefully I don't break it this time.) |
1e3de1a
to
d0b0a2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I just have one suggested change to style/formatted!
This commit adds some basic 16bpp support for BMP File Format. Added support for reading and using of 16 bit mask values from file. All values are scaled to ARGB255 format based on bit depth of source color channel. Removed warning, it's no longer required as 16 bit with alpha bit(s) are now supported. Adjusted spacing, added spacing and brackets to make clang static check happy.
56cede7
to
e74a0ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes seem fine to me. Should be good to merge this pending CI passing. Great work!
Thanks! And congrats for your first merged Godot contribution 🎉 |
This commit adds some basic 16bpp support for BMP File Format.
Doesn't support R5G6B5 pixel format yet as I haven't figured out how to detect it's usage seperatly from the A1R5G5B5/X1R5G5B5 pixel formats from the available header information. Should be easy to add support for it though once that's nailed down.
Doesn't support Alpha bit yet. This is because while it seems to still have it's transparency correct when I open test images in Gimp, I have no such luck in Godot and I'm not sure why. I think some relevant information maybe being stored in the colorspace or the V5 BMP header that I'm either ignorant of or have missed.
Edit: Added spacing and line breaks for legability.