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.

Sign up for GitHub

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

Jump to bottom

64 bit compilation #36

Open
GrimSqueaker opened this issue Feb 8, 2021 · 52 comments
Open

64 bit compilation #36

GrimSqueaker opened this issue Feb 8, 2021 · 52 comments
Labels
enhancement New feature or request

Comments

@GrimSqueaker
Copy link
Owner

GrimSqueaker commented Feb 8, 2021 •
edited
Loading

I will start with trying to build the code in 64 bit. In order to get going, I will to the same thing as with the Linux build by introducing conditional compilation with "FIXME" notes and printouts. In my previous clone of the repo I got to the first level with that approach but it only could render the map and not the 3d world. I think if we consequently move forward with structures and rewrite those pointer to int assignments we can get it working at some point.

For this I have introduced the preprocessor define COMPILE_FOR_64BIT that can be set in CMake by passing -DUSE_64_BIT=True on the command line. It would be good if somebody could also introduce this into the VS solution, since I am not proficient in VS.

The benefit of this preprocessor flag is that I can leave the 32bit version unchanged and after having it compile with 64 bit can start to iteratively only fix those code locations that are triggered when starting to play the game.
@GrimSqueaker GrimSqueaker added the enhancement New feature or request label Feb 8, 2021
@turican0
Copy link
Collaborator

After pull your code my Visual Studio 2019 generates this erros, I will be find problems, Tom.
image

@turican0
Copy link
Collaborator

turican0 commented Feb 10, 2021 •
edited
Loading

MEMSET: identificator not found
image
#error: Macro definition of vsnprintf conflicts with Standard Library function declaration
image

@turican0
Copy link
Collaborator

Problem is in 5fb70fe Fix: Fix Cmake after splitting files

@turican0
Copy link
Collaborator

When I remove engine/defs.h from sub_main.h it is ok.(defs.h is already included by Basic.h), Tom.

@turican0
Copy link
Collaborator

I actualize Visual Studio settings to run 64bit version, game drop after begin - in sound initialization:
image

@GrimSqueaker
Copy link
Owner Author

Sorry, I noticed the MS build failing yesterday but it was too late in the evening for me to fix it.

I had problems yesterday merging my 64bit branch, because it had conflicts with the Bit -> int_t changes as well as the introduction of separate files. Now I am back with 212 compilation errors in GCC, which I will also comment out via the preprocessor, before I will start to fix my way into the main menu. This should be possible and not too hard. I had this already working in my previous copy of the repo. What comes after this, I don't know. Your array->structs changes probably help a lot to get further.

@turican0
Copy link
Collaborator

In addition to rewriting fields into structures, we will also have to write structure converters that contain pointers, they must be converted when loading or saving data (usually saves). I would put them in the special FilesConvert.cpp / .h files. The simplest will be to have a copy of the data structure, where they will be instead of int32_t pointers. Raw data would be loaded into them, and then copied to a pointer structure. (Recorded pointers are most often zero)

typedef
{
uint8_t a
uint8_t* b
}
structureX

typedef
{
uint8_t a
uint32_t b
}
shadowX

LoadWithConvertX:
LoadFromFile(shadow,file)
structureX.a=shadowX.a
structureX.b=shadowX.b

or

SaveWithConvertX:
shadowX.a=structureX.a
shadowX.b=structureX.b
SaveToFile(shadow,file)

Tom.

@GrimSqueaker
Copy link
Owner Author

Yes, just by compiling as 64 bit the structure that is saved as in-game save (type_D41A0_BYTESTR_0) increases in size from 224791 to 245115. Some of that might be fixed by using int32_t instead of int, etc. But for other stuff we will need converters.

I am not yet as proficient as you with all the existing structures. In order to prevent me from breaking essential stuff, I have implemented for this particular structure a unit test that is now also running as part of the GitHub Linux CI action. If you have more structures that should never change in size or other constraints that we definitely have to keep, please feel free to enhance the unit tests.

If I find time, I could also try to make the CMake setup - including unit tests - run on Windows. That would be a big improvement if we could use the same build system. But it would change the way you and @thobbsinteractive are working. It would mean that the VS solution is not checked in to the repository anymore but instead before you can work on the code you would have to generate it via CMake. I understand that this complicates some things but simplifies others at the same time. For pure Windows developers this is a seemingly unnecessary step, but for example it would get rid of the multiple checked in build directories (CMake would prepare all necessary files when generating the solution), it would allow to generate for XCode on MacOS in the future, and much more.

@thobbsinteractive
Copy link
Collaborator

thobbsinteractive commented Feb 13, 2021 via email

@GrimSqueaker
Copy link
Owner Author

In a lot of places int32_t would help with the 64 bit build. But a naive find-and-replace would also make the code cumbersome to read, because simple loop counters etc. can safely stay int. So, I guess, I would just change it, where it actually has to be the correct size. I have started with some fixes for that.

Higher game resolution would be a great enhancement. It also seems that that the game speed depends on the window resolution. If I start it in 640x480 (game and window) everything is unplayable fast. If I use 1920x1080 as window resolution with scaling the game feels just about right.

Yes, it should be possible to open the CMakeLists.txt as if was is a solution and Visual Studio does some magic to hide the behind-hte-scenes from you. But in some places the CMake setup probably is too Linux specific until I generalise it.

@thobbsinteractive
Copy link
Collaborator

thobbsinteractive commented Feb 13, 2021 via email

@turican0
Copy link
Collaborator

Hi
I would automatically select int on int32_t only in structures, Elsewhere it has to be done manually.
I don't know if I can break CMake in a visual studio. We can try it, if it doesn't work I can always copy * .sln and * .vcproj from older revisions.
Game frame looks like this:
-compute all events
-draw all entites
-if it was faster than the set value, wait

In other words, the game rendering is slow and SDL rendering output too. Mybe we can optimize slow pixel by pixel rendering in VGA_Blit in port_sdl_vga_mouse.cpp.

Tom.

@thobbsinteractive
Copy link
Collaborator

thobbsinteractive commented Feb 13, 2021 via email

@GrimSqueaker
Copy link
Owner Author

Hi Tim,
did you check in the enhancedassets directory? Seems like it is missing at the moment?
I will adjust CMake to use the new layout.
Cheers,
Sebastian

@thobbsinteractive
Copy link
Collaborator

thobbsinteractive commented Feb 13, 2021 via email

@thobbsinteractive
Copy link
Collaborator

thobbsinteractive commented Feb 13, 2021 via email

@GrimSqueaker
Copy link
Owner Author

Thanks for the quick fix. :-)

@turican0
Copy link
Collaborator

I fix many problems for x64 compilation. For next work I must write convert mechanism for translate array<>struct save files,
(otherwise I would damage load save fixes for original game compactibility - this is necessarily for debugging)
Tom.

@thobbsinteractive
Copy link
Collaborator

thobbsinteractive commented Feb 18, 2021 via email

@GrimSqueaker
Copy link
Owner Author

Hi,

thanks for the 64bit work, Tom. But the current PR crashes in 32bit when starting the first level. I had no time to investigate further today (because of the Perseverance landing live stream :-). Maybe tomorrow I can take a closer look. It crashes in DataFileIO::Read with a strange "malloc(): unsorted double linked list corrupted" error. This seems to origin in glibc with either a heap overflow or more likely a heap corruption by writing to unwanted addresses. I guess, I will have to bisect the git commits to find the change that introduced the regression.

Tell me if you need help setting stuff up on Linux, Tim. I like helping people and I am glad getting some feedback and improving the Linux CMake setup so that it not just "works for me".

Cheers,
Sebastian

@turican0
Copy link
Collaborator

Hi Sebatian, I watched Perseverance landing too, and I wonder what he can do :)
In my system is starting first level in 32bit ok, please try to find out the details, or correct the error if you find the cause.
Maybe anythink with sub_83E80_freemem4 or x_DWORD_E9C38_smalltit(backing up this buffer to other variables is sometimes weird).
Tom.

@turican0
Copy link
Collaborator

You can compare code with origin disassembled code in remc2-dev/remc2/memimages/NETHERW.c for finding errors, too.
Tom.

@thobbsinteractive
Copy link
Collaborator

thobbsinteractive commented Feb 19, 2021 via email

@turican0
Copy link
Collaborator

I found out that I get a memory error when switching the resolution 640-> 320, I'll try to decide and maybe the problem will disappear for you, Tom.

@turican0
Copy link
Collaborator

I test two implementation of void sub_83E80_freemem4(uint8_t* ptr)//264e80
1:
if (ptr != NULL)
{
free((void)*ptr);
ptr = NULL;
}
and 2:
free((void)ptr);

Both runs with errors.

At now is better remove this code. Later we can rewrite malloc/free to malloc_safety/free_safety with simple garbage collector.
(or change the whole allocation method)

See if it works now.

Tom.

@GrimSqueaker
Copy link
Owner Author

With your latest development branch I can now start the first level again (in 32bit), but it crashes when I build a castle. I'll try to investigate.
If you are interested, this is the callstack:
Screenshot from 2021-02-20 21-27-12
I would like to find a solution for this before I merge it, because otherwise I might not be able to test things out anymore.

Yes, the 32bit ARM might behave quite differently, but I have never tried it. You can check the list of dependencies from the Linux CI which is running on Ubuntu. And both, Ubuntu and Raspbian, are based on Debian. So, I would assume that the packages are identically named.

The next Perseverance live stream is announced for 22 Feb at 8:00 CET. Maybe there will be some video available already. Currently, Madrid is receiving 2Mb/s from MRO probably used as a relay (https://eyes.nasa.gov/dsn/dsn.html). This is so cool.

@GrimSqueaker
Copy link
Owner Author

Ah, sorry. My mistake. I should have checked the code first. It is triggered by the preprocessor flag "TEST_x64". I will disable this on Linux and merge the PR.

@turican0
Copy link
Collaborator

turican0 commented Feb 20, 2021 •
edited
Loading

This is ok, allert_error is only my margin for problematic part of code(when game gone to this, game crash, and we can analyze/fix it). I look at it.

In x64 I have made progress. Particly rendered frame of game(frame before full fade in):
image

@turican0
Copy link
Collaborator

Hi Tim, interlaced render is not visible, may be game this feature turn on only on slow machines(in original code, in remc2 is set best quality by default). Or can't be this code needed for blur effect.?
Tom.

@turican0
Copy link
Collaborator

turican0 commented Feb 21, 2021 •
edited
Loading

Begin of first level in x64 playable!!!
image

@thobbsinteractive
Copy link
Collaborator

thobbsinteractive commented Feb 21, 2021 via email

@turican0
Copy link
Collaborator

x64 mostly fixed. I finish level 1 and play level 2 without errors. At now we must play whole game for bug finding, Tom.

@GrimSqueaker
Copy link
Owner Author

Wow. Now I have to get this working on Linux, because the compiler is much more picky in what is valid code. But as compared to what you have achieved making it work on Linux is a minor issue.

This VR support sounds cool.

@GrimSqueaker
Copy link
Owner Author

I stand corrected. It is working right away. The first level is completely playable in 64bit on Linux. Amazing work and thanks to you, Tom. 👍

This raises the question: Will we need 32 bit in the future? At least for CMake it would simplify things, if I remove special cases and only compile for the system native bitness. It would complicate Linux CI if we insist on verifying 32bit compatibility.

@turican0
Copy link
Collaborator

32-bit compatibility will probably always be useful for easier debugging, it may not be available on all platforms, but at least for me it is important for 32-bit windows.

@GrimSqueaker
Copy link
Owner Author

Hi Tom,

while working on deleting dead code I stumbled upon this code

				v63 = (char*)x_InterlockedExchange((long*)&v64, (signed __int32)(v57 + 1));
				*v63 = v62;

in sub_63670_draw_minimap_a which is only called in 320 resolution. The problem is that x_InterlockedExchange always returns 0 and is dereferenced. This might explain your crash in 320.

In other news, I have now removed around 23k lines of dead code that has not been called from anywhere. I will bring this to the dev branch soon.

Cheers,
Sebastian

@thobbsinteractive
Copy link
Collaborator

thobbsinteractive commented Feb 23, 2021 via email

@turican0
Copy link
Collaborator

Good job Seb, for x_InterlockedExchange, can use this code?
(I read https://docs.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedexchange)
We maybe must change long to other type.

long x_InterlockedExchange(long volatile* Target, long Value)
{
long temp = *Target;
*Target = Value;
return temp;
};
Tom.

@turican0
Copy link
Collaborator

turican0 commented Feb 24, 2021 •
edited
Loading

I fix x64 in sub_63670_draw_minimap_a, but in level 1 this function not used, I test it later,
Tom.

@GrimSqueaker
Copy link
Owner Author

GrimSqueaker commented Feb 24, 2021 •
edited
Loading

Ok, would have been a lucky try. Is there anything threaded going on that really requires locks? Because otherwise we could just remove this.

Edit: By "remove this" I mean "replace it by a standard assignment".

@turican0
Copy link
Collaborator

We don't need locks, it just wants to test if it works well, and then simplify it (remove the x_InterlockedExchange).
But it's not important now, it only makes sense in terms of code cleanup (which would like to cleanup comprehensively anyway).
Tom.

@thobbsinteractive
Copy link
Collaborator

thobbsinteractive commented Feb 27, 2021 via email

@turican0
Copy link
Collaborator

Hi Tim,
good job.
You could re-attach the screenshot for a better understanding? I don't see it in your post,
Thanks, Tom.

@Tim-Hobbs
Copy link

Hi Tom, I replied via Email I guess that is why it did not appear here. When I fix the menus etc... I'll send a better one.
1080_Screen

@GrimSqueaker
Copy link
Owner Author

Hey, this looks great. Good job. And on the way you probably learned a lot to untangle some of the rendering code an data structures.

@thobbsinteractive
Copy link
Collaborator

thobbsinteractive commented Feb 28, 2021 via email

@turican0
Copy link
Collaborator

turican0 commented Mar 1, 2021

It is super!!!
Would it help us to disassemble Populous III code? It should be based on the same engine and may use DirectX / OpenGL, but I'm not sure.
Tom.

@Tim-Hobbs
Copy link

Tim-Hobbs commented Mar 1, 2021 via email

@turican0
Copy link
Collaborator

turican0 commented Mar 2, 2021

I add popTB.c and D3DPopTB.c - disassempled Populous III to memimages, you can inspire it for creation D3D or OpenGL rendering,
Tom.

@Tim-Hobbs
Copy link

Thanks Tom I will take a look once I have sorted out the in game menus.
I have discovered that the slow frame rate is due to the Software Render. We already double buffer with SDL.

I can hopefully quickly fix this if I use the interlacing code I found for the VR render and multi-thread the render with one extra thread to draw alternate rows. Obviously I would make the multi-threading render a configurable and option.

Do you guys have any suggestions for multi-platform threading libraries or will the standard C++ 11 ones work? As I remember in Windows you can force the OS to dedicate a cpu-core to a thread rather than allowing the OS to decide where it is executed. I am wondering if that is possible in a multi-platform libraries?

@GrimSqueaker
Copy link
Owner Author

I think for portability I would go with C++11 on-board threading capabilities like https://en.cppreference.com/w/cpp/header/thread
But to be honest, I did not use it yet apart of a simple hello world kind of program. So I don't know if it can handle what you have in mind.

@Tim-Hobbs
Copy link

I will look into it thanks

GrimSqueaker pushed a commit that referenced this issue Jun 14, 2022
@thobbsinteractive
Merge pull request #36 from thobbsinteractive/Spiderwebs-hitting-play…
8d1b3be 
…ers-are-in-left-corner

Spiderwebs hitting players are in left corner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@thobbsinteractive @GrimSqueaker @turican0 @Tim-Hobbs