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

[rtext] Fix default font alpha on Big Endian systems #4624

Merged
merged 2 commits into from
Dec 23, 2024

Conversation

Fancy2209
Copy link
Contributor

@Fancy2209 Fancy2209 commented Dec 20, 2024

Alpha was broken for rtext's default font on Big Endian
Before
imagem
After
imagem

@raysan5
Copy link
Owner

raysan5 commented Dec 20, 2024

@Fancy2209 It seems this patch addresses a specific use case for a specific hardware arquitecture... won't endianness affect other parts of raylib library? 🤔

@Fancy2209
Copy link
Contributor Author

I tested some examples and they work, but I still have to test loading assets

Also it isn't just PowerPC/PowerPC64
image-6.png

image-7.png

I'll search for further issues with Big Endian
By the way, it isn't just PowerPC that's big endian, it's just the more common one

@Peter0x44
Copy link
Contributor

Big endian is uncommon, but worth supporting. This will break all big endian systems, like powerpc linux, and sparc linux. Not that they are common, but it still matters to some users.

@Fancy2209
Copy link
Contributor Author

Big endian is uncommon, but worth supporting. This will break all big endian systems, like powerpc linux, and sparc linux. Not that they are common, but it still matters to some users.

You mean this issue affects them too or that this PR breaks them?

@Peter0x44
Copy link
Contributor

I mean the issue affects them. I think sparc and powerpc are the most relevant BE systems

@raysan5
Copy link
Owner

raysan5 commented Dec 20, 2024

@Fancy2209 @Peter0x44 what is the cost of supporting big endian in raylib? It can require many changes and additional code complexity for a very specific set of platforms... if it implies only a couple of checks, it can be considered...

@Fancy2209
Copy link
Contributor Author

Fancy2209 commented Dec 20, 2024

I have to test all examples and find issues so it might take a bit
According to an article Peter0x44 code should always be endian independent anyways so it shouldn't be too hard to patch
I'll do a testing spree of all examples in a bit, fix the broken one and report back once I do so we can evaluate the complexity, though it shouldn't be very hard from my understanding

@raysan5
Copy link
Owner

raysan5 commented Dec 20, 2024

@Fancy2209 ok, nice! 👍😄

@Peter0x44
Copy link
Contributor

Peter0x44 commented Dec 20, 2024

It implies no extra complexity, just a few minor edge cases need some extra care and attention
This case can actually be rewritten not to need the ifdef by writing both bytes of the short separately
Something like:

((unsigned char *)imFont.data)[(i + j)*sizeof(short)] = 0xFF
((unsigned char *)imFont.data)[(i + j)*sizeof(short) + 1] = 0x00;

@Fancy2209
Copy link
Contributor Author

Fancy2209 commented Dec 21, 2024

It implies no extra complexity, just a few minor edge cases need some extra care and attention This case can actually be rewritten not to need the ifdef by writing both bytes of the short separately Something like:

((unsigned char *)imFont.data)[(i + j)*sizeof(short)] = 0xFF
((unsigned char *)imFont.data)[(i + j)*sizeof(short) + 1] = 0x00;

Done, I still have to test loading Data from files though

@raysan5
Copy link
Owner

raysan5 commented Dec 23, 2024

@Fancy2209 please, let me know when ready! did you test some examples to see other possible related point to review?

@raysan5 raysan5 changed the title Fix rtext default font alpha on Big Endian [rtext] Fix default font alpha on Big Endian systems Dec 23, 2024
@Peter0x44
Copy link
Contributor

This change only applies to loading the default font and is fine on its own.
There might be other issues with endianness to review but there is no reason not to merge this one.

@raysan5 raysan5 merged commit 7868d60 into raysan5:master Dec 23, 2024
14 checks passed
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.

3 participants