Convert endianess upon read to support BE #8091
Conversation
|
|
OpenRCT2/src/openrct2/util/Endian.h Line 5 in 8c66047 That and I would like to know if it really helps. What do you mean by "Why macros over functions"? Can you provide an example? |
|
Can you do this without using |
There was a problem hiding this comment.
I'm not sure if this is worthwhile, considering only legacy systems (and some highly specialised ones) are big endian. It's also very easy to break, as none of us use it (apart from Kenton, who actually has a BE system lying around?).
I'm not going to go as far as vetoing it, but it's borderline.
I don't understand this negativity. This PR introduces compatibility with an entirely new series of architectures, which will help in potentially porting the game to the Wii, for example. Moreover, it does so in a relatively non-intrusive way: essentially 48 lines are changed. If this works as well as advertised, it has my blessing. |
I think it has very, very limited use - nobody is going to play OpenRCT2 on the Wii. As it stands, Android barely works, and that's not likely to improve. Why increase the development burden for something nobody (or almost nobody) is going to use? It means that Janisozaur will now need to watch import code closely to check for any changes breaking big endian system, even though he doesn't seem to have such a system available (again, because nobody uses them any more). Again, I'm not vetoing since apparently I'm in the minority and I'm not going to veto stuff on a whim. |
|
I would rather vote for having "ReadValue" return the actual correct value regardless on what platform you are on, forgetting it just once will cause bugs. |
|
@ZehMatt This was actually my initial approach, but things start to fall apart in cases like |
|
Yeah that makes sense, one way is to specialize the templates, so in case of FileIndexHeader it would go over the members, I know its slightly more work but at the end no one has to deal with this anymore. |
|
FYI, It's not just the Wii. PS3 (Which runs Linux if you want it to), XBOX 360, there could be BigEndian variants of RiscV in the future, as well as newer PowerPC Hardware (Being Sold Today) being able to switch easily, at any time. (And I think the 3DS as well?). I will admit that with most of these being last gen, and current gen mostly being x86, it doesn't matter as much as it once did, when the next gen consoles were likely to be PowerPC based. I'm not saying we should increase code complexity if it hurts maintainability, though. If it can be done like ZehMatt says, as I understand it, that would probably be the best route. Take no weight with my words. Edit: How does Locomotion do it? |
|
Sure, it makes it possible to run on consoles, but who is going to do that? An Android version is way more viable, but even that is neglected to death. We could use our manpower way more efficiently than trying to chase platforms people won't use. But since BE support is going to be merged anyway: has anyone actually tested this on BE hardware? |
There was a problem hiding this comment.
Like @ZehMatt has suggested, the conversion should be done in the reader / writer either by wrapping the file stream with an adaptor or passing a flag. The edge cases where structs are directly written can be done on a case by case basis, most of that is limited to local-only files anyway like the caches.
|
Note that there's also a new POWER9 desktop / server available (Talos II from Raptor Engineering). |
|
Sure, but who's going to run OpenRCT2 on that? Also, doesn't the new POWER hardware support little endian mode anyway? |
|
|
I asked for BE support almost a year ago in the forums. Gymnasiast responded there with same agumentation then he did here. @AaronVanGeffen I agree to you that his opinions about niche systems are very negative. Our PowerMac's would benefit from this BE support,too. What many don't know, we can still build SDL2 for OSX Leopard PowerPC. It just needs a few fixes and no offical support ofc :) Thanks to the people who acutally work on BE support, no matter what others say. I'm following this thread now, still learning C tho, hope to get better in future to deliver proper fixes to this project. |
|
Just if anyone cares, OpenRCT2 0.2.1 with this patch builds on FreeBSD 12.0-RELEASE on POWER: |
|
@pkubaj What causes it to not build without this patch? |
| # else | ||
| # define RCT2_ENDIANESS __ORDER_BIG_ENDIAN__ | ||
| # define HIBYTE(w) ((uint8_t)(w)) | ||
| # define LOBYTE(w) ((uint8_t)(((uint16_t)(w) >> 8) & 0xFF)) |
There was a problem hiding this comment.
@IntelOrca these are required, I added #error in case RCT2_ENDIANESS is not defined and HIBYTE and LOBYTE are used somewhere.
…o 0.2.2, fix build with big-endian architectures, change maintainer Patch for big-endian compatibility taken from upstream at OpenRCT2/OpenRCT2#8091 PR: 226897 Approved by: mat (mentor) Differential Revision: https://reviews.freebsd.org/D19941 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@500708 35697150-7ecd-e111-bb59-0022644237b5
…o 0.2.2, fix build with big-endian architectures, change maintainer Patch for big-endian compatibility taken from upstream at OpenRCT2/OpenRCT2#8091 PR: 226897 Approved by: mat (mentor) Differential Revision: https://reviews.freebsd.org/D19941
…o 0.2.2, fix build with big-endian architectures, change maintainer Patch for big-endian compatibility taken from upstream at OpenRCT2/OpenRCT2#8091 PR: 226897 Approved by: mat (mentor) Differential Revision: https://reviews.freebsd.org/D19941 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@500708 35697150-7ecd-e111-bb59-0022644237b5
…o 0.2.2, fix build with big-endian architectures, change maintainer Patch for big-endian compatibility taken from upstream at OpenRCT2/OpenRCT2#8091 PR: 226897 Approved by: mat (mentor) Differential Revision: https://reviews.freebsd.org/D19941 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@500708 35697150-7ecd-e111-bb59-0022644237b5
|
Closing this for now. It might get resumed at some point in the future, but I don't feel like working on this now. |
|
I have a PS3 which happens to be a big endian ppc64 of the Power7 variety with SPU, and I am willing to compile and test on it. |
|
@grepwood thanks for your offer, porting to PS3 (assuming the game OS) would first require confirming all the libraries work fine. There's little interest in big endian port and while personally I would love to see it done, I just can't find it rewarding enough to do. It's not even a question of platform availability at this stage, I was able to run and improve our tests for big endian using qemu, but never really got around to fixing them. I fear just contributing platform time is not enough to make this happen. |
|
@janisozaur just to get 1 thing out of the way - I would not count on a GameOS port anytime soon. I have already broke my back trying to port ec-/Quake3e and we found out the SDK has a mostly missing network stack. But a Linux port? It's totally doable. |
|
Would you be willing to work on such port? |
|
You bet! Aaron from Discord already said to fork from develop branch. I'm on it. |
|
Sounds great. You can use this PR if you find it useful. Ping me on discord as well, I might be able to collaborate a bit too. |
…o 0.2.2, fix build with big-endian architectures, change maintainer Patch for big-endian compatibility taken from upstream at OpenRCT2/OpenRCT2#8091 PR: 226897 Approved by: mat (mentor) Differential Revision: https://reviews.freebsd.org/D19941
ping @Krutonium