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 fixes #135

Merged
merged 5 commits into from
Aug 29, 2022
Merged

Big endian fixes #135

merged 5 commits into from
Aug 29, 2022

Conversation

BSzili
Copy link
Contributor

@BSzili BSzili commented Aug 23, 2022

This could be useful for porting Dethrace to XB360, PS3, Wii consoles, or perhaps more obscure platforms.

@madebr
Copy link
Collaborator

madebr commented Aug 23, 2022

Thanks! On what platforms have you tested this?

  • Instead of doing #if defined(__m68__) || defined(__ppc__), I would use the TestBigEndian CMake module. That way we don't need to maintain a list of big endian architectures.
  • There is also endian dependent code here, concerning the savegames.

@classilla
Copy link

Yes, please don't just defined(__ppc__) -- there are little-endian PowerPC systems too. Raptor POWER9s can swing both ways (my Talos II is little-endian).

@BSzili
Copy link
Contributor Author

BSzili commented Aug 23, 2022

It looks like TestBigEndian is deprecated, can CMAKE_C_BYTE_ORDER be used here? The save game code should be easy to fix, I'll make SWAP32_BE a no-op if BR_ENDIAN_BIG is 1.

@madebr
Copy link
Collaborator

madebr commented Aug 23, 2022

That's fine to me.
Supporting older CMake versions is not a priority here.

@dethrace-labs. WDYT? CMake 3.20 is widely available by now.

Don't forget to bump the the minimum required version.

@BSzili
Copy link
Contributor Author

BSzili commented Aug 23, 2022

Just to clarify I'm not very familiar with CMake, so I don't insist on using the newer method. I'll look into whichever way you say would suit the project better.

@dethrace-labs
Copy link
Owner

Yep, I like the use of CMAKE_C_BYTE_ORDER. I'm fine to take a dependency on cmake 3.20. Thanks for the PR!

@BSzili
Copy link
Contributor Author

BSzili commented Aug 24, 2022

I'm not at my usual setup so I tried to use WSL2 to test my changes. It turns out Ubuntu Bionic only has CMake 3.10.2 so I tried to use the older method. I added this to the root CMakeLists.txt, but it doesn't seem to change the generated Makefile:

include(TestBigEndian)
test_big_endian(IS_BIGENDIAN)
if (IS_BIGENDIAN)
    add_compile_definitions(BR_ENDIAN_BIG=1)
else()
    add_compile_definitions(BR_ENDIAN_LITTLE=1)
endif()

Maybe I put this in the wrong place?

@madebr Sorry, I was so preoccupied with CMake that I forgot to answer your original question. I tested this on the Amiga.

@madebr
Copy link
Collaborator

madebr commented Aug 24, 2022

Maybe I put this in the wrong place?

Because you're using add_compile_options (and not target_compile_options(<target> PUBLIC <compile_optoins>)),
you need to do the add_compile_options BEFORE any add_library calls.
Also, it needs be put AFTER the first call to project.
So somewhere in this range.

@madebr Sorry, I was so preoccupied with CMake that I forgot to answer your original question. I tested this on the Amiga.

Nice. Tested on a real Amiga or an emulated one?
Is OpenGL support done through software emulation?

@BSzili
Copy link
Contributor Author

BSzili commented Aug 24, 2022

Maybe I put this in the wrong place?

Because you're using add_compile_options (and not target_compile_options(<target> PUBLIC <compile_optoins>)), you need to do the add_compile_options BEFORE any add_library calls. Also, it needs be put AFTER the first call to project. So somewhere in this range.

I see, thanks. I'll try fiddle around with this after work.

@madebr Sorry, I was so preoccupied with CMake that I forgot to answer your original question. I tested this on the Amiga.

Nice. Tested on a real Amiga or an emulated one? Is OpenGL support done through software emulation?

Only in emulation for now, the lack of software renderer is a big roadblock. I cobbled together some code to draw wireframe models, but it's quick just a test, very buggy and slow. I'm still contemplating what to do about this, as OpenGL is not an option on 25 year old hardware.

@dethrace-labs
Copy link
Owner

that sounds awesome! Could you post a couple of screenshots!?

@BSzili
Copy link
Contributor Author

BSzili commented Aug 26, 2022

It's nothing to write home about, but here's one where it's not immediately obvious that I didn't do any clipping:

@dethrace-labs
Copy link
Owner

looks awesome :)

@dethrace-labs
Copy link
Owner

dethrace-labs commented Aug 29, 2022

I'm not a CMake expert at all, but heres what I tried:

In the top-level CMakelists.txt, I added

include(TestBigEndian)
test_big_endian(IS_BIGENDIAN)

above find_package(SDL2 REQUIRED)

In BRSRC13/CMakeLists.txt, I added

if(IS_BIGENDIAN)
    target_compile_definitions(brender PRIVATE BR_ENDIAN_BIG)
else()
    target_compile_definitions(brender PRIVATE BR_ENDIAN_LITTLE)
endif()

In DETHRACE/CMakeLists.txt:

if(IS_BIGENDIAN)
    target_compile_definitions(dethrace PRIVATE BR_ENDIAN_BIG)
else()
    target_compile_definitions(dethrace PRIVATE BR_ENDIAN_LITTLE)
endif()

A similar pattern should be added to S3/CMakeLists.txt.

It seems to set the value correctly as seen by the pre-processor. Maybe give that a try?

@dethrace-labs
Copy link
Owner

dethrace-labs commented Aug 29, 2022

May I post your screenshot on the https://twitter.com/dethrace_labs twitter feed?

@BSzili
Copy link
Contributor Author

BSzili commented Aug 29, 2022

Of course, if you think it's interesting feel free to post it. I'll rebase the PR and implement the changes you have suggested.

@dethrace-labs dethrace-labs merged commit 20c2128 into dethrace-labs:main Aug 29, 2022
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.

4 participants