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

Big endian and amigaos4 support #5190

Closed
wants to merge 2 commits into from
Closed

Conversation

afxgroup
Copy link
Contributor

This pull requests allow big endian systems to use ImgUi
And related to this added also amigaos4 support

@ocornut
Copy link
Owner

ocornut commented Apr 12, 2022

Thank you for the PR!
For the color layout I think we should instead simply use a

#ifndef IM_COL32_R_SHIFT    
..
#endif 

In order to allow any combination (IMGUI_USE_BGRA_PACKED_COLOR is a bit arbitrary), so you would define it in your imconfig file. It's not sure a matter of endianness but the desired color format may vary for other reasons (and may also be adjusted in shader or render state configuration).

@afxgroup
Copy link
Contributor Author

afxgroup commented Apr 13, 2022

No problem :) LITTLE/BIG_ENDIAN allows any platform to automatically adapt the code but your solution is ok for me and i don't have finally patch every time imgui to be used on my amiga projects :)
If you tell me how you think to use IM_COL32_R_SHIFT i'll change the file

@ocornut
Copy link
Owner

ocornut commented Apr 13, 2022

If you tell me how you think to use IM_COL32_R_SHIFT i'll change the file

Just enclose the block with _SHIFT and_MASK declaration under e.g.

#ifndef IM_COL32_R_SHIFT  // User can declare their own format

block that's all it needs.

@afxgroup
Copy link
Contributor Author

afxgroup commented Apr 13, 2022

Something like this? (without tabs if you don't like them)

`

#ifdef IMGUI_USE_BGRA_PACKED_COLOR

#ifndef IM_COL32_R_SHIFT  
	#define IM_COL32_R_SHIFT    16  
	#define IM_COL32_G_SHIFT    8  
	#define IM_COL32_B_SHIFT    0  
	#define IM_COL32_A_SHIFT    24  
	#define IM_COL32_A_MASK     0xFF000000  
#endif  

#else

#ifndef IM_COL32_R_SHIFT  
	#define IM_COL32_R_SHIFT    0  
	#define IM_COL32_G_SHIFT    8  
	#define IM_COL32_B_SHIFT    16  
	#define IM_COL32_A_SHIFT    24  
	#define IM_COL32_A_MASK     0xFF000000  
#endif  

#endif
`

(sorry for bad code display but github do something weird with multiple #ifdefs)

@PathogenDavid
Copy link
Contributor

(sorry for bad code display but github do something weird with multiple #ifdefs)

You need to use ``` instead of a single ` without nesting or anything.

For example: (Click to expand)
```cpp
#ifdef IMGUI_USE_BGRA_PACKED_COLOR
	#ifndef IM_COL32_R_SHIFT  
		#define IM_COL32_R_SHIFT    16  
		#define IM_COL32_G_SHIFT    8  
		#define IM_COL32_B_SHIFT    0  
		#define IM_COL32_A_SHIFT    24  
		#define IM_COL32_A_MASK     0xFF000000  
	#endif  
#else
	#ifndef IM_COL32_R_SHIFT  
		#define IM_COL32_R_SHIFT    0  
		#define IM_COL32_G_SHIFT    8  
		#define IM_COL32_B_SHIFT    16  
		#define IM_COL32_A_SHIFT    24  
		#define IM_COL32_A_MASK     0xFF000000  
	#endif
#endif
```

becomes

#ifdef IMGUI_USE_BGRA_PACKED_COLOR
	#ifndef IM_COL32_R_SHIFT  
		#define IM_COL32_R_SHIFT    16  
		#define IM_COL32_G_SHIFT    8  
		#define IM_COL32_B_SHIFT    0  
		#define IM_COL32_A_SHIFT    24  
		#define IM_COL32_A_MASK     0xFF000000  
	#endif  
#else
	#ifndef IM_COL32_R_SHIFT  
		#define IM_COL32_R_SHIFT    0  
		#define IM_COL32_G_SHIFT    8  
		#define IM_COL32_B_SHIFT    16  
		#define IM_COL32_A_SHIFT    24  
		#define IM_COL32_A_MASK     0xFF000000  
	#endif
#endif

@ocornut
Copy link
Owner

ocornut commented Apr 13, 2022

So how about not duplicating an ifndef that doesn't need to be duplicated ?

ocornut pushed a commit that referenced this pull request Apr 13, 2022
@ocornut
Copy link
Owner

ocornut commented Apr 13, 2022

Squashed, tweaked and merged as 14ca75d
Thank you.

@ocornut ocornut closed this Apr 13, 2022
@DanielGibson
Copy link
Contributor

I think it would still be nice if default IM_COL32_*_SHIFT and IM_COL32_A_MASK defines were provided for both little and big endian, so the user only has to #define IMGUI_BIG_ENDIAN or something like that.
(Of course it would be even better if the user didn't have to do anything, but AFAIK it's impossible to detect endianess at compiletime in C/C++ code in a portable way, at least without shittons of platform-specific checks)

DanielGibson added a commit to DanielGibson/dhewm3 that referenced this pull request Oct 30, 2024
DanielGibson added a commit to DanielGibson/dhewm3 that referenced this pull request Oct 31, 2024
DanielGibson added a commit to DanielGibson/dhewm3 that referenced this pull request Oct 31, 2024
@afxgroup
Copy link
Contributor Author

afxgroup commented Nov 1, 2024

Well, all modern C library has already something to detect if the system is big or little endian at compile time. It is not so impossible

@ocornut
Copy link
Owner

ocornut commented Nov 1, 2024

You can just #define IMGUI_USE_BGRA_PACKED_COLOR.
I don't think it has to do with system endianness however, it's a matter of framebuffer/color encoding endianness, therefore I don't think it would be correct to alter the color layout based on system endianness.

@DanielGibson
Copy link
Contributor

DanielGibson commented Nov 1, 2024

You can just #define IMGUI_USE_BGRA_PACKED_COLOR

I haven't tried it (and I don't have a Big Endian machine myself so I'd have to ask dhewm3 users to test it), but as far as I can tell from the code it wouldn't help (or even work on any platform, when using OpenGL without modifying the backend)?

It does not change any settings in the rendering backends (for how to interpret color values or whatever; Update: Except for the DX9 backend), the only thing it does is redefining IM_COL32_*_SHIFT for BGRA instead of RGBA - while for big endian it would have to be ABGR.

Apparently it does depend on system endianess, as it's always the Big Endian users who complain that the colors are wrong (both in the dhewm3 bugtracker and, as far as I can tell, the ImGui bugtracker) - though it might also depend on the used rendering API.

I think the general problem is that Dear ImGui views pixels/colors as 32bit unsigned integer, where Red is in the 8 least significant bits and alpha in the 8 most significant bits - but apparently the rendering APIs (at least OpenGL with GL_RGBA and GL_UNSIGNED_BYTE) view a RGBA pixel as 4 bytes with R in the first byte and A in the last byte - so when feeding it uint32_t data, how it ends up looking is endian-specific.

Update: I briefly looked at other rendering backends.

  • DX10-12 use DXGI_FORMAT_R8G8B8A8_UNORM, which is "A four-component, 32-bit unsigned-normalized-integer format that supports 8 bits per channel including alpha." - so that should work regardless of endianess because it assumes uint32_t pixels - though I'm not 100% sure if I interpret that correctly (is it "32bit integer" or "32bit format with 8bit unsigned integer channels"?) - not that it matters, AFAIK DX10+ only exists on Little Endian platforms and apparently there it's byte-wise RGBA.
  • DX9 is the only backend with special handling for IMGUI_USE_BGRA_PACKED_COLOR and uses D3DFMT_A8B8G8R8, but I couldn't tell from the documentation whether it interprets the data byte-wise or as uint32_t. (This is the only DX version that exists on a Big Endian platform, the xbox360)
  • OpenGL2 and 3 use GL_RGBA with GL_UNSIGNED_BYTE => byte-wise RGBA, causing endian issues with ImU32 colors
  • Vulkan uses VK_FORMAT_R8G8B8A8_UNORM which is "a four-component, 32-bit unsigned normalized format that has an 8-bit R component in byte 0, an 8-bit G component in byte 1, an 8-bit B component in byte 2, and an 8-bit A component in byte 3" => if I interpret this correctly, it's byte-wise RGBA, like OpenGL, causing endian issues with ImU32 colors
  • Metal uses MTLPixelFormatRGBA8Unorm (for the fonts texture at least) which also seems to assume uint32_t (but Metal doesn't matter as it only exists on Little Endian platforms)
  • SDL2 and SDL3 use SDL_PIXELFORMAT_ABGR8888 which should be a uint32_t format with 8bits/pixel in ABGR order, but "high bit -> low bit", so the 8 most significant bits are Alpha and the 8 least significant bits are Red, just like in default IM_COL32.
    But SDL also interpret the color data from the vertices as SDL_Color * which is basically struct { uint8_t r; uint8_t g; uint8_t b; uint8_t a; }; i.e. byte-wise RGBA data => causing endian issues with ImU32 colors

@DanielGibson
Copy link
Contributor

Well, all modern C library has already something to detect if the system is big or little endian at compile time. It is not so impossible

Not impossible, but at least a PITA, as it's not standardized in the C standard:

  • POSIX has endian.h with BYTE_ORDER which can be compared to LITTLE_ENDIAN and BIG_ENDIAN, but:
    • on Linux that's just <endian.h>
    • on the BSDs it's apparently <sys/endian.h>
    • on macOS / Mac OSX (and probably iOS) it seems to be <machine/endian.h> - which should at least define BYTE_ORDER, LITTLE_ENDIAN and BIG_ENDIAN, but not the standard POSIX endian conversion functions like htobe16()
    • no idea WTF the other Unix and Unix-like systems (Solaris, QNX, AIX, ...) do
  • I couldn't find any equivalent defines for Windows
    • one could always assume Little Endian on Windows (except if targeting xbox360)
  • Who knows what proprietary Console SDKs use

@afxgroup
Copy link
Contributor Author

from stackoverflow:

#include <Windows.h>

#if REG_DWORD == REG_DWORD_LITTLE_ENDIAN
# define le_to_host_ulong(VAL) VAL
# define be_to_host_ulong(VAL) _byteswap_ulong(VAL)
# define le_to_host_ushort(VAL) VAL
# define be_to_host_ushort(VAL) _byteswap_ushort(VAL)
# define le_to_host_uint64(VAL) VAL
# define be_to_host_uint64(VAL) _byteswap_uint64(VAL)
#else
# define le_to_host_ulong(VAL) _byteswap_ulong(VAL)
# define be_to_host_ulong(VAL) VAL
# define le_to_host_ushort(VAL) _byteswap_ushort(VAL)
# define be_to_host_ushort(VAL) VAL
# define le_to_host_uint64(VAL) _byteswap_uint64(VAL)
# define be_to_host_uint64(VAL) VAL
#endif```

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

Successfully merging this pull request may close these issues.

4 participants