Marzac / le3d

A straightforward and easy to use 3D software renderer for real-time retro graphics.
https://marzac.github.io/le3d/
MIT License
60 stars 6 forks source link

Amiga support #18

Closed m0ppers closed 6 years ago

m0ppers commented 6 years ago

Initial amiga support. cube example works. No mouse/keyboard/joystick interactions but a good starting ground. If this is ok I will create a pull request for the AMMX stuff shortly

Marzac commented 6 years ago

Hallo m0ppers.

So plenty of things to say.

First of all, many thanks for your great work. It's an impressive contribution and I hope this will help to make this library popular and interesting for people to develop with.

Second, my sincere apologize for having misspelled your family name, I am correcting this in all the files where the mistake can be found.

Now comes the hard part.

Marzac commented 6 years ago

So, in order of criticality.

1) Information for CMake (targets and options)

You introduced a new target (the Amiga) and new flags for CMake to work. Should not we have a readme / INSTALL file that explains all the user configurable flags available to compile the library and examples? Maybe with some default command lines devs can "copy and paste" to quickly try out the executables?

Marzac commented 6 years ago

2) Information on toolchain

I have no idea which compilers / toolchain one should use to compile for Amiga targets. I don't know what toolchains you did try with the codebase. Should it be mentioned somewhere (with download links, packets required, advices to install ...)

Marzac commented 6 years ago

3) BmpFile implementation

From a software architecture standpoint, I don't like what is happening in BmpFile class right now. This class belongs to the generic / all-platforms part of the engine and according to my original design, it should not have any architecture specific code.

I completely understand what and why you did that but there is a more generic approach to this known problem. I can suggest you a new version of this class tonight. It will instead parse the header byte per byte in a separate function and assemble words so the compiler handles the endianness by himself.

Marzac commented 6 years ago

4) Pre-processor compilation flags

I saw you prevent definition of flags depending on the architecture, namely: LE_USE_SIMD
LE_USE_SSE

And annihilated the warning messages (in geometry.h) ! (That's terrible, common :-) )

This is not this project codebase philosophy, to me, any build configuration flag MUST to be defined. They can be either set to 0 or 1. I prefer not seeing ANY #ifdef or #if defined (except for recursive inclusion prevention). You can always prevent the effect of the flag on special cases.

Here is an example:

geometry.h

if LE_USE_SIMD == 1

#ifdef __GNUC_
    #include "geometry_simd.h"
#else
    #include "geometry_scalar.h"
#endif

else

#include "geometry_scalar.h"

endif // LE_USE_SIMD

It forces to use the scalar classe, even though the LE_USE_SIMD is set, for compilers different than GCC.

The reason behind this is to make sure that compilation process is totally known and understood by the application developer and make sure there are no file inclusion problems. Like one forget to include "config.h" in some file and parts of the engine will be compiled with SIMD support, others not, which will result in an unpredictable mess that maybe compiles but will crash at runtime.

Marzac commented 6 years ago

5) Older compiler support

During the development of the engine core, I tried to stick to a very limited subset of the C++ to make sure the code is highly portable and compatible. Which means C++ 98, C library, no STL, no exceptions, no RTI, no virtuals, static allocation (except for runtime loadable resources).

This ensures the codebase can be built for tiny micro-controllers, systems without an OS (with little modifications) and older machines. I think this compatibility is very important for this project and also for people who do retrodev with antic / deprecated toolchains.

I see you are using an "auto" statement in your code (L77, draw_amiga.cpp). If you use the real type instead, I believe all your code will be compatible with C++98 or lower.

Marzac commented 6 years ago

That's what I can tell for now. Once more, congratulations!

m0ppers commented 6 years ago

We are getting closer :D

1+2: Yes I was thinking the same thing. However I didn't want to clutter the README.md. I can create a separate BUILDING.md file or we add more sections to the README.md. What do you think?

  1. Yes this was and is totally ugly :D I couldn't come up with a more elegant solution quickly and as it is quite isolated I thought it wouldn't hurt for the moment. If you have a better solution it would be awesome :)

  2. Ok. My thoughts were "Neither SSE nor SIMD have any real meaning on the amiga so they should probably not be set.". As I removed them the warning had to go as well. Thought it was a left over when maybe the config.h file didn't exist or so.

That being said the config.h file is bothering me right now as it is under source control. In my amiga branches I always have to enable the int rasterizer and then I have a locally modified source file. So it is not really configurable. The cmake way to do this would be to add the config stuff as individual options (https://cmake.org/cmake/help/v3.0/command/option.html) and then do a configure_file (https://cmake.org/cmake/help/v3.0/command/configure_file.html) to generate a config.h from cmake. However that is of course quite a change. Not sure if this would be desired? I can create a pull request if it is. So far I can live with the config.h changes.

  1. Too much c++11 recently (I really love auto ;) ). Sorry. Will fix.
Marzac commented 6 years ago

1+2) Very good idea for the BUILDING.md, let's do this! I'll add all the required info for various versions of Windows and Linux if you do the Amiga and MacOS

3) I know how to implement it quickly, probably done by tomorrow.

4) Excellent point. I made several time the mistake to commit a modified and not reverted to default config.h file after some tests. The cmake "options" sound appropriate to me. We can then provide a default configuration set per platform and not having to resort to undefined preprocessor symbols. It's a good solution I think.

5) No personal problem with C++11, I just want to maintain a lower standard for susmentionned reasons.

Have a great day!

Marzac commented 6 years ago

@m0ppers

I pushed a new version of LeBmpFile with my "vision" of how to deal with the endianness problem. Please have a look to see if it works great with the Amiga platform.

I have a doubt on one thing: Why do the component bit masks have to be converted from little-endian to big-endian?

Sounds like a mystery to me and may result in the swapping of the components in the color structure from little endian to big endian platforms and this is weird to me.

Anyway, the bitmap import / export functions seem to work as expected on little endian systems. Have a nice evening.