hhyyrylainen / Leviathan

Leviathan Game Engine
Other
17 stars 8 forks source link

Refactoring random stuff #32

Open crodnu opened 5 years ago

crodnu commented 5 years ago

What it says in the tin, refactorng random crap to learn the engine internals while doing something marginally useful.

hhyyrylainen commented 5 years ago

This broke some tests (I didn't review the code yet) : at least the quaternion math is broken.

hhyyrylainen commented 5 years ago

Doesn't compile on linux (circleci build fails):

[ 44%] Building CXX object Engine/CMakeFiles/Engine.dir/Common/StartEndIndex.cpp.o
/root/leviathan/Engine/Common/StartEndIndex.cpp: In function 'std::ostream& operator<<(std::ostream&, const Leviathan::StartEndIndex&)':
/root/leviathan/Engine/Common/StartEndIndex.cpp:8:12: error: no match for 'operator<<' (operand types are 'std::ostream' {aka 'std::basic_ostream<char>'} and 'const char [2]')
     stream << "[";
     ~~~~~~~^~~~~~
/root/leviathan/Engine/Common/StartEndIndex.cpp:6:25: note: candidate: 'std::ostream& operator<<(std::ostream&, const Leviathan::StartEndIndex&)' <near match>
 DLLEXPORT std::ostream& operator<<(std::ostream& stream, const StartEndIndex& value)
                         ^~~~~~~~
/root/leviathan/Engine/Common/StartEndIndex.cpp:6:25: note:   conversion of argument 2 would be ill-formed:
/root/leviathan/Engine/Common/StartEndIndex.cpp:8:15: error: invalid user-defined conversion from 'const char [2]' to 'const Leviathan::StartEndIndex&' [-fpermissive]
     stream << "[";
               ^~~
In file included from /root/leviathan/Engine/Common/StartEndIndex.cpp:1:
/root/leviathan/Engine/Common/StartEndIndex.h:34:11: note: candidate is: 'constexpr Leviathan::StartEndIndex::StartEndIndex(size_t)' <near match>
 constexpr StartEndIndex::StartEndIndex(size_t start) noexcept : Start(start) {}
           ^~~~~~~~~~~~~
/root/leviathan/Engine/Common/StartEndIndex.h:34:11: note:   conversion of argument 1 would be ill-formed:
/root/leviathan/Engine/Common/StartEndIndex.cpp:8:15: error: invalid conversion from 'const char*' to 'size_t' {aka 'long unsigned int'} [-fpermissive]
     stream << "[";
               ^~~
/root/leviathan/Engine/Common/StartEndIndex.cpp:8:15: error: invalid conversion from 'const char*' to 'size_t' {aka 'long unsigned int'} [-fpermissive]
In file included from /root/leviathan/Engine/Common/StartEndIndex.cpp:1:
/root/leviathan/Engine/Common/StartEndIndex.h:34:47: note:   initializing argument 1 of 'constexpr Leviathan::StartEndIndex::StartEndIndex(size_t)'
 constexpr StartEndIndex::StartEndIndex(size_t start) noexcept : Start(start) {}
                                        ~~~~~~~^~~~~
/root/leviathan/Engine/Common/StartEndIndex.h:27:25: note: candidate: 'std::ostream& Leviathan::operator<<(std::ostream&, const Leviathan::StartEndIndex&)' <near match>
 DLLEXPORT std::ostream& operator<<(
                         ^~~~~~~~
/root/leviathan/Engine/Common/StartEndIndex.h:27:25: note:   conversion of argument 2 would be ill-formed:
/root/leviathan/Engine/Common/StartEndIndex.cpp:8:15: error: invalid user-defined conversion from 'const char [2]' to 'const Leviathan::StartEndIndex&' [-fpermissive]
     stream << "[";
               ^~~
In file included from /root/leviathan/Engine/Common/StartEndIndex.cpp:1:
/root/leviathan/Engine/Common/StartEndIndex.h:34:11: note: candidate is: 'constexpr Leviathan::StartEndIndex::StartEndIndex(size_t)' <near match>
 constexpr StartEndIndex::StartEndIndex(size_t start) noexcept : Start(start) {}
           ^~~~~~~~~~~~~
/root/leviathan/Engine/Common/StartEndIndex.h:34:11: note:   conversion of argument 1 would be ill-formed:
/root/leviathan/Engine/Common/StartEndIndex.cpp:8:15: error: invalid conversion from 'const char*' to 'size_t' {aka 'long unsigned int'} [-fpermissive]
     stream << "[";
               ^~~
/root/leviathan/Engine/Common/StartEndIndex.cpp:8:15: error: invalid conversion from 'const char*' to 'size_t' {aka 'long unsigned int'} [-fpermissive]
In file included from /root/leviathan/Engine/Common/StartEndIndex.cpp:1:
/root/leviathan/Engine/Common/StartEndIndex.h:34:47: note:   initializing argument 1 of 'constexpr Leviathan::StartEndIndex::StartEndIndex(size_t)'
 constexpr StartEndIndex::StartEndIndex(size_t start) noexcept : Start(start) {}
                                        ~~~~~~~^~~~~
In file included from /usr/include/c++/8/bits/basic_string.h:48,
                 from /usr/include/c++/8/string:52,
                 from /root/leviathan/Engine/Define.h:16,
                 from /root/leviathan/Engine/Common/StartEndIndex.h:3,
                 from /root/leviathan/Engine/Common/StartEndIndex.cpp:1:
/usr/include/c++/8/string_view:545:5: note: candidate: 'template<class _CharT, class _Traits> std::basic_ostream<_CharT, _Traits>& std::operator<<(std::basic_ostream<_CharT, _Traits>&, std::basic_string_view<_CharT, _Traits>)'
     operator<<(basic_ostream<_CharT, _Traits>& __os,
     ^~~~~~~~
/usr/include/c++/8/string_view:545:5: note:   template argument deduction/substitution failed:
/root/leviathan/Engine/Common/StartEndIndex.cpp:8:15: note:   mismatched types 'std::basic_string_view<_CharT, _Traits>' and 'const char*'
     stream << "[";
               ^~~
In file included from /usr/include/c++/8/string:52,
                 from /root/leviathan/Engine/Define.h:16,
                 from /root/leviathan/Engine/Common/StartEndIndex.h:3,
                 from /root/leviathan/Engine/Common/StartEndIndex.cpp:1:
/usr/include/c++/8/bits/basic_string.h:6314:5: note: candidate: 'template<class _CharT, class _Traits, class _Alloc> std::basic_ostream<_CharT, _Traits>& std::operator<<(std::basic_ostream<_CharT, _Traits>&, const std::__cxx11::basic_string<_CharT, _Traits, _Allocator>&)'
     operator<<(basic_ostream<_CharT, _Traits>& __os,
     ^~~~~~~~
/usr/include/c++/8/bits/basic_string.h:6314:5: note:   template argument deduction/substitution failed:
/root/leviathan/Engine/Common/StartEndIndex.cpp:8:15: note:   mismatched types 'const std::__cxx11::basic_string<_CharT, _Traits, _Allocator>' and 'const char [2]'
     stream << "[";
               ^~~
/root/leviathan/Engine/Common/StartEndIndex.cpp:10:16: error: ambiguous overload for 'operator<<' (operand types are 'std::ostream' {aka 'std::basic_ostream<char>'} and 'const long unsigned int')
         stream << value.Start.value();
         ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~

(and so on, I didn't copy paste everything)

Also there is a warning (which I assume means you haven't ran clang format on the files you have edited):

/root/leviathan/Engine/Utility/ComplainOnce.cpp: In member function 'bool Leviathan::ComplainOnce::ThreadUnsafeComplainOnce::wasErrorLogged(const string&)':
/root/leviathan/Engine/Utility/ComplainOnce.cpp:37:5: warning: this 'for' clause does not guard... [-Wmisleading-indentation]
     for(auto iter = FiredErrors.begin(); iter != FiredErrors.end(); ++iter)
     ^~~
/root/leviathan/Engine/Utility/ComplainOnce.cpp:40:2: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'
  return false;
  ^~~~~~
crodnu commented 5 years ago

\o/

hhyyrylainen commented 5 years ago

I have now reviewed the actual code and first of there are some good changes that I would merge if they were in a separate commit (so I could cherry-pick it): these are the removal of the maths files that never got written. Then there are some good ideas like the usage of std::optional and making warn once thread safe, unfortunately these are implemented in a way that I don't like them. There should be no calls to .has_value() and the StringIterator debugging is most likely broken due to the change. And the extra class of having a Monitor is not good. Everything that is thread safe should inherit from ThreadSafe and use GUARD_LOCK();. Then there are the vector types, which I think have been split into too many files and even with splitting Types.h into 3 files is in my opinion quite unnecessary. Also the fact that the implementations of the functions for these classes are outside the class is not consistent with the style I want to go for. And I also think that the nan check debugging help is broken here as well. So in summary I'm not going to accept this pull request as is.