LauriM commented 9 years ago

I am placing the code review as an issue for now, as there are bigger chunk of code checked at once.

Thirdparty folder structure

Looks like the catch is not on its own folder. Keeping the "include/lib/etc" folders in the subfolders of the dependencies is a better idea, to make it easier in the future.

I would propose the following layout:

thirdparty/catch 
thirdparty/catch/include/*includes*
thirdparty/somelib/include/*includes*
thirdparty/somelib/libs/*libs*

This way its possible to support libraries that require more custom stuff. (Requiring specific layout, etc, can be kept in the subdirectories) Its also easier to keep different dependencies separated.

It adds some extra work when adding new dependencies (you need to add the folders to the project files manually) but that should not be a problem.

Android implementation

Seems to go with the right way. Using the "NativeActivity" c++ stuff is kinda useless, as Google fucked up it. Its better to just go with custom Java and do the required bindings yourself, its easier that way in the end.

So, don't use the C++ NativeActivity, just code the required Java, its currently more stable way of doing things.

Overall project structure

Overall project structure/split seems good so far.

Vectors&Matrix

Vector/Matrix operations shoud be inlined!

Int types

Custom inttypes are excellent idea, however, they should be wrapped per platform.

Make somekind of header file / define flag to detect a platform
Apply the inttypes/etc per platform. (Some odd platforms may want different types)
Coding style

Just a personal note, this hurts my eyes:

    Viewport::Viewport(const uint32 x, const uint32 y, const uint32 width, const uint32 height) : x(x),
                                                                                                  y(y),
                                                                                                  width(width),
                                                                                                  height(height) {
    }

But yeah, not a thing to focus if thats the way you have decided to go.

Filemanager

Way too much virtuality & header defining!

How about the following, shared header "FileManager.h" that has header file with the methods, etc. Then FileManagerWin32.cpp and FileManagerUnix.cpp, that are only compiled per platform. (Header guards, or not included or something)

This way you don't have extra cost of virtual functions, no inheritance, etc. And you get the same functionality required!

Also, I would maybe name this class as "FileSystem", "FileManager sounds more like something that actually manages the files, etc, above the FileSystem.

Simple virtual filesystem should also be considered.

My personal implementation of this as reference:

By a quick look it seems like the graphics wrapper is going to have way too much inheritance and virtual functions.

I would recommend a shared header and separated implementations per platform. This removes the requirement for excessive inheritance and virtual functions. For different data I would recommend opaque pointers.

Depending on the overall idea of the engine design, DOD style containers for the data/resources could be highly effective.

Unittests

Testing seems to be fine, keep on going on that

Conclusion

The project seems to be on a good start. If you have any question, etc. Just contact me and I am more than happy to explain my ideas and give more recommendations.

It seems like the engine design is heavily OOP style, with heavy inheritance and virtual functions. I would recommend against heavy OOP usage, as it can be really problematic during the later optimizations for performance.

Babelz commented 9 years ago

Hi

Thanks about the feedback! It's nice to have to feedback once in a while, i really appreciate it.

About the graphics wrapper: first i had the idea to use inheritance and virtual functions to implement the whole system. Few days ago, i did some researching and now the graphics wrapper will be implemented using the same style as you proposed. Single header, many source files.

About DOD: i have been prototyping, trying to get familiar with the principles and just playing around with DOD principles for about a two weeks now, i can now see why it is such a big deal while dealing with high performance demanding applications. DOD principles will definitely be used while developing the engine.

Babelz

siquel commented 9 years ago

Did the changes you suggested to FileSystem (previous FileManager). It was way easier with only one header. Also inlined vectors/matrices. Next step should be to support Windows Phone, OSX/iOS bundles and Android asset manager. I think I'll focus first on iOS/OSX because i don't have Hyper-V installed so i cant test Android and WP yet. We may need to include some Objective-C glue, though it can be done with C if i recall correctly, need to do some researching first. OSX uses the same kind of implementation as Linux, but we need to add support for bundles which is the way Apple wants to distribute apps, so i think we could do common implementation for few unix platforms, for example:

#if SANI_TARGET_PLATFORM != SANI_PLATFORM_WIN32 && SANI_TARGET_PLATFORM != SANI_PLATFORM_WP8
bool isAbsolutePath(const String& path) const { 
// non-windows stuff
}
bool openFile(const String& path, const Filemode mode) {
 // fopen stuff
}
// and so on
#endif

And then implement the platform specific functions to another file. Does that sound little 'hacky'? Or we could use inheritance?

Babelz / SaNi

CodeReview #4