std::variant-less version?

[viciious]

Hi,

thanks for your on animation support for libRocket! Unfortunately due to various operating systems, libs and compilers still catching up with C++17 features, compiling your fork is some real pain. The most problematic bit is std::variant and std::visit, the later is only available in MacOS 10.14 and llvm installed from brew.

Now please forgive my ignorance, but was using std::variant really necessary or maybe there's a way to get rid of it at the expense of a few additional lines of code?

Thanks.

[mikke89]

Hi,

thanks for checking out the library! With regards to std::variant, I actually think the use of it here is a very good fit: a closed set of types, each with slightly varying data and a few methods distinct for each type.

I don't see myself investing time into this, unfortunately. If you want to have a go at this yourself, I think the quickest approach would be tagged unions for std::variant and switches for std::visit. Of course, an objective oriented approach could also be considered. However, I'm afraid it will suffer in terms of performance which I'd argue is important here, due to pointer indirection and virtual lookup calls. It's a shame that C++17 should be an issue today.

[viciious]

The issue is even more pronounced if you consider various CI systems such as travis, which are typically lagging behind in providing the latest available tools and libraries.

[viciious]

Another alternative would be to re-implement std::visit, which is the main source of trouble as far as cross-platform compatibility is concerned.

[mikke89]

I'd prefer to have a separate branch for C++14 support (or even C++11), rather than going in-between standards and partly reimplementing the standard library.

Do you have something working for yourself now?

[mikke89]

Alright, I did the thing! Have a look at the cpp11 branch. Only tested on MSVC, does it work for you?

[barotto]

Hi, I'm trying to compile the cpp11 branch with gcc 5.4.0 (ubuntu 16.04).

The first problem I had was

Include/Rocket/Core/Vector2.inl:80:13: error: ‘roundf’ is not a member of ‘std’

which I resolved including <cmath> in Vector.h

Now I'm stuck with this error:

Include/Rocket/Core/Matrix4.inl:71:10: error: returning a value from a constructor
  return *this;
          ^

Last time I checked constructors can't return anything, hence the error, but I don't know c++17 at all so maybe this return statement is allowed in that spec level? Or maybe MSVC doesn't care? Is it needed or can I safely remove it?

PS: thanks for your dev work on libRocket! I think you should rename your fork in libMissile or something and publish it as a new indipendent project. libRocket is (was?) the only good open source GUI middleware based on HTML/CSS.

[mikke89]

Hi!
Thanks for the report. I don't know why MSVC allows the return statement, it was probably just copy-pasted from the assignment operator, and can safely be removed.

If you encounter additional problems, I'd happily receive a pull request :)

Hah, I like the idea of libMissile. It's a good idea, although I'm a bit reluctant because it indicates a dedication to it that I'm just now willing to take yet. I'll consider it. The changes are quite substantial now, so it does make sense.

[barotto]

It seems like there are some other C++14 and above only stuff in the code.

Source/Core/Element.cpp:2647:24: error: ‘make_unique’ is not a member of ‘std’
      transform_state = std::make_unique<TransformState>();

make_unique is from C++14, I've included a Cpp11compat.h file with a make_unique implementation in Core.h to solve this.

Source/Core/PropertyParserAnimation.cpp: In function ‘bool Rocket::Core::ParseTransition(Rocket::Core::Property&, const StringList&)’:
Source/Core/PropertyParserAnimation.cpp:229:51: error: no matching function for call to ‘Rocket::Core::TransitionList::TransitionList(<brace-enclosed initializer list>)’
  TransitionList transition_list{ false, false, {} };
                                                   ^
In file included from Source/Core/../../Include/Rocket/Core/Core.h:34:0,
                 from Source/Core/precompiled.h:31,
                 from Source/Core/PropertyParserAnimation.cpp:29:
Include/Rocket/Core/Animation.h:62:8: note: candidate: Rocket::Core::TransitionList::TransitionList()
 struct TransitionList {
        ^
Include/Rocket/Core/Animation.h:62:8: note:   candidate expects 0 arguments, 3 provided
Include/Rocket/Core/Animation.h:62:8: note: candidate: Rocket::Core::TransitionList::TransitionList(const Rocket::Core::TransitionList&)
Include/Rocket/Core/Animation.h:62:8: note:   candidate expects 1 argument, 3 provided
Include/Rocket/Core/Animation.h:62:8: note: candidate: Rocket::Core::TransitionList::TransitionList(Rocket::Core::TransitionList&&)
Include/Rocket/Core/Animation.h:62:8: note:   candidate expects 1 argument, 3 provided

In C++11 you can't initialize a struct that has default member initializers from a braced-init-list. This limitation has been removed in C++14.
I removed the initializers in TransitionList just to proceed but I don't expect it to be a good solution...

Now libRocketCore.so compiles but has many linking errors, all related to Vector3::Normalise(), such as:

CMakeFiles/RocketCore.dir/Source/Core/Box.cpp.o: in function "Rocket::Core::Vector3<float>::Normalise() const":
Box.cpp:(.text+0x0): multiple definitions of "Rocket::Core::Vector3<float>::Normalise() const"
CMakeFiles/RocketCore.dir/Source/Core/BaseXMLParser.cpp.o:BaseXMLParser.cpp:(.text+0x110): defined here for the first time

As for the pull request, I already have a libRocket fork so I'm not entirely sure how GitHub will handle the situation. Other that that, I need to compile a working lib first and then test it. Is there any example code I can use to test the Animation part?

Thanks again!

PS: seriously think about the "libMissile" idea, it would be a shame if all this work is forgotten and lost :( documentation is already in markdown format and I can expand it with your additions...

[viciious]

Now libRocketCore.so compiles but has many linking errors, such as:

This is weird as these are coming from template specialization

[viciious]

Maybe we should be aiming for c++14 as the requirement instead of c++11?..

[barotto]

Now libRocketCore.so compiles but has many linking errors, such as:

This is weird as these are coming from template specialization

When you fully specialize something, it doesn't depend on a template parameter any more, so you need to put it in a .cpp file instead of a .h or you end up violating the one definition rule.
There's a Rocket::Core::Vector3<float>::Normalise() definition inside Vector3.inl, I'll put it in a .cpp file and see what happens.
Strange that MSVC doesn't complain. But it said nothing about the return inside a constructor either, so...

[barotto]

In the end adding inline in the function definition was enough.
Now everything compiles and links.
I'll prepare a pull request.

[mikke89]

Yeah, we had the same template specialization problem on Vector2::Round, I'm not sure why MSVC is more relaxed on this.

Also, MSVC doesn't really have a C++11 mode, which is why the make_unique worked for me. It seems everything works now, so I'll close this issue.

I'll open a new issue on the libMissile idea :)

[viciious]

@barotto @mikke89 thanks!