Add ability for user to override container types #110

rokups · 2020-07-11T07:56:48Z

This is an implementation of my ideas raised in #107. Please review and comment.

It was nice to find out that lua integration did not require any changes whatsoever. Yay!

Few unrelated observations:

Project should adopt .editorconfig. I had to restore bunch of trailing spaces to avoid extra noise in commits. Editor was eager to get rid of them. This would also configure editors to use tabs.
Project should adopt .gitattributes and normalize line endings. Most files use \n while some use \r\n. This puts us in a position where one line change may flag entire file as changed only because editor changed line endings.
Changes to CMakeLists.txt are not proper, but then again entire build script is in a state where it just can not be fixed, but should be rewritten instead. I might do it in the future (no promises).

rokups · 2020-07-12T07:31:04Z

Now that build is fixed and i have some time - i should talk about stinks of this PR.

In order to make RmlUi work with EASTL i had to remove final specifier from Releaser class. This sucks a little bit, and as a workaround it could at least be configurable. Problem is that type aliases depend on contents of Traits.h header, but at the same these aliases and supposed config define #define RMLUI_RELEASER_FINAL final would be in UserConfig.h which then should be included before Traits.h. We have a chicken and egg problem. So far i completely removed final to make it build, but we may want to address this in one way or the other.

Second thing i do not really like is UserConfig.h name. It is not so much a config, but rather type aliases only, so maybe we should name it UserTypes.h? And maybe still add UserConfig.h that would be included very early and contain defines like RMLUI_RELEASER_FINAL or RMLUI_NO_THIRDPARTY_CONTAINERS. It would be a small config at this point, but at least we could include this header early enough.

Let me know how you would like to handle this.

mikke89 · 2020-07-12T17:50:41Z

Thank you for the PR! Looks very good so far.

Few unrelated observations:

[....]

I think it would be good to discuss these things in a new issue.

In order to make RmlUi work with EASTL i had to remove final specifier from Releaser class. This sucks a little bit, and as a workaround it could at least be configurable. Problem is that type aliases depend on contents of Traits.h header, but at the same these aliases and supposed config define #define RMLUI_RELEASER_FINAL final would be in UserConfig.h which then should be included before Traits.h. We have a chicken and egg problem. So far i completely removed final to make it build, but we may want to address this in one way or the other.

Looks like this is just for the Releaser class. Can't we just forward declare it? Ideally, the config file shouldn't have any dependencies.

Second thing i do not really like is UserConfig.h name. It is not so much a config, but rather type aliases only, so maybe we should name it UserTypes.h? And maybe still add UserConfig.h that would be included very early and contain defines like RMLUI_RELEASER_FINAL or RMLUI_NO_THIRDPARTY_CONTAINERS. It would be a small config at this point, but at least we could include this header early enough.

I'm a bit worried that splitting the config and types would make the CMake settings a bit overwhelming and make it harder to navigate the configuration. I'd prefer just the name Config.h since it should also provide a default configuration. Maybe we could put it in a subfolder as well <RmlUi/Config/Config.h>? Then I could add a Version.h file there later.

I think the new CMake settings should only be enabled and displayed if a bool option is first enabled. This could also hide the NO_THIRDPART_CONTAINERS option and warn (if enabled) that it has no effect when using custom type config.

I'm in the process of moving this coming week, so a bit busy, but I will try to find some time here and there to go through it and review it in more detail.

mikke89

Some minor additional comments.

Include/RmlUi/Core/DataModel.h

mikke89 · 2020-07-13T09:23:49Z

Include/RmlUi/Core/DataModel.h

@@ -168,7 +169,7 @@ class RMLUICORE_API DataModelConstructor {
 	// Register an array type.
 	// @note The type applies to every data model associated with the current Context.
 	// @note If 'Container::value_type' represents a non-scalar type, that type must already have been registered with the appropriate 'Register...()' functions.
-	// @note Container requires the following functions to be implemented: size() and begin(). This is satisfied by several containers such as std::vector and std::array.
+	// @note Container requires the following functions to be implemented: size() and begin(). This is satisfied by several containers such as Vector and Array.


I'd rather keep the original here.

mikke89 · 2020-07-13T09:28:44Z

Include/RmlUi/Core/Types.h

-using Matrix4f = ColumnMajorMatrix4f;
+using Matrix4f = RMLUI_DEFAULT_MATRIX_TYPE;
+
+}	// namespace Rml


Not sure why the namespace is closed and then opened?

mikke89 · 2020-07-14T13:39:12Z

Samples/basic/bitmapfont/src/FontEngineBitmap.cpp

@@ -33,7 +33,7 @@

 namespace FontProviderBitmap
 {
-	static std::vector<std::unique_ptr<FontFaceBitmap>> fonts;
+	static Rml::Vector<std::unique_ptr<FontFaceBitmap>> fonts;


Missing UniquePtr (maybe do a find all on the samples?)

Include/RmlUi/UserConfig.h

mikke89 · 2020-07-14T13:56:18Z

Include/RmlUi/UserConfig.h

+#define RMLUI_COLOUR_USER_EXTRA
+// Extra code to be inserted into RmlUi::Vector2<> class body.
+#define RMLUI_VECTOR2_USER_EXTRA
+// Extra code to be inserted into RmlUi::Vector3<> class body.
+#define RMLUI_VECTOR3_USER_EXTRA
+// Extra code to be inserted into RmlUi::Vector4<> class body.
+#define RMLUI_VECTOR4_USER_EXTRA
+// Extra code to be inserted into RmlUi::Matrix4<> class body.
+#define RMLUI_MATRIX4_USER_EXTRA


We should comment out these optional defines in the default configuration.

Also, it would be nice with an example in comments, at least for one of these options.

mikke89 · 2020-07-18T12:58:27Z

Include/RmlUi/UserConfig.h

+
+// Following defines should be used for insering custom type cast operators for conversion of RmlUi types
+// to user types. RmlUi uses template math types, therefore conversion operators to non-templated types
+// should be done using SFINAE as in example below.


Thanks for the examples. I must admit all the SFINAE hurts my eyes a lot. I'd prefer if the examples assumed the user types are templated, and tell them to use SFINAE with perhaps a single SFINAE example for eg. vector2.

The color example is a bit problematic, because float colors are usually interpreted in linear color space, while byte versions are usually in SRGB. At least this is the assumption used internally in RmlUi. I don't think we ever use the float color in any public API though. By the way they should be divided by 255 and not 256 :)

Are you sure about the example? SFINAE is horrible. However duty of the library is to make people's lives easier. More often than not i bump into engines with non-templated math types, but for the sake of argument we can say there is about equal amount of engines with either implementation. Now if engine is using templated types then it would be trivial to implement type conversion operators, ignoring SFINAE junk here. But if engine uses non-templated types then writing this stuff takes some effort, especially for people not familiar with these arcane arts. I think it would be much more helpful if we left an extensive example for something that is quite a common case.

The color example is a bit problematic,

Hmm i do not have an answer to that, other than adding a comment warning to mind differences of colorspaces.

By the way they should be divided by 255 and not 256 :)

Nobody saw that 😊

Alright, you have me convinced :) If I get too miserable looking at those examples after a while I might move them to the documentation later. The color part is fine like that.

mikke89 · 2020-07-19T07:55:26Z

How do you feel this is coming along? We should probably try to merge it as soon as possible so it doesn't diverge too much with new commits.

What do you think about the cmake and filename suggestions in #110 (comment) ?

rokups · 2020-07-19T08:17:30Z

Looks like i only missed adding NO_THIRDPART_CONTAINERS to CMake and i did not try forward-declaring releaser. Other than that this seems to me like good enough for merge. Did i miss anything else?

mikke89 · 2020-07-19T08:23:10Z

I was thinking about these suggestions as well:

I'd prefer just the name Config.h since it should also provide a default configuration. Maybe we could put it in a subfolder as well <RmlUi/Config/Config.h>? Then I could add a Version.h file there later.

I think the new CMake settings should only be enabled and displayed if a bool option is first enabled.

rokups · 2020-07-19T08:26:32Z

That makes sense yes. Will do.

I tried forward-declaring Releaser. Outcome is bit hairy. This introduces a dependency on configuration header when Traits.h is included. So we must always include Types.h before/instead of Traits.h. Traits.h can not be included alone. Unless that header itself also included config header. Not ideal.

Include/RmlUi/UserConfig.h

Include/RmlUi/Core/Types.h

mikke89 · 2020-07-19T08:39:18Z

I tried forward-declaring Releaser. Outcome is bit hairy. This introduces a dependency on configuration header when Traits.h is included. So we must always include Types.h before/instead of Traits.h. Traits.h can not be included alone. Unless that header itself also included config header. Not ideal.

Alright, if you prefer I'm fine with splitting the configuration into Config.h and ConfigTypes.h or TypeConfig.h as you suggested earlier.

rokups · 2020-07-19T09:44:21Z

Maybe we could look into making some common include that would pull in header.h/config/types/traits headers. They all are basic building blocks after all.

…

On July 19, 2020 11:39:31 "Michael R. P. Ragazzon" ***@***.***> wrote: I tried forward-declaring Releaser. Outcome is bit hairy. This introduces a dependency on configuration header when Traits.h is included. So we must always include Types.h before/instead of Traits.h. Traits.h can not be included alone. Unless that header itself also included config header. Not ideal. Alright, if you prefer I'm fine with splitting the configuration into Config.h and ConfigTypes.h or TypeConfig.h as you suggested earlier. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

mikke89 · 2020-07-19T10:39:50Z

Yeah that's a good idea. We have some headers that need to be included everywhere, such as Header.h, Platform.h.

We could add a Common.h header that includes all of these things, including the config, traits, and types. We should try to keep it as small as possible though for compile times sake.

rokups · 2020-07-21T06:33:56Z

Moved configuration into RmlUi/Config/Config.h and added a new Common.h for including common headers. Tbh i still think this stinks somewhat. Common.h is not that common due to legacy split of RmlUi to 3 dlls (why it was done i have no idea 🤔 ). We also have Core/Header.h and Lua/Header.h. To get Core/Header.h we can include Core/Common.h, to get Lua/Header.h we have to include just that. I do not think Lua/Header.h should exist at all tbh. This is a build time decision. We may want to drop Lua/Header.h and move RmlUi/Core/Common.h to RmlUi/Common.h to make it library-global instead of something that core module owns (if that makes any sense).

… from size_t.

Resolves mikke89#107.

Co-authored-by: Michael R. P. Ragazzon <mikke89@users.noreply.github.com> Update Include/RmlUi/Core/Types.h Co-authored-by: Michael R. P. Ragazzon <mikke89@users.noreply.github.com>

mikke89 · 2020-07-21T10:53:44Z

Hmm, I'm fine with having it as it is, I think it's a bit weird to include any Lua stuff if you don't use it.

I think all headers should be able to be included separately though, so I'd like to see Types.h and the others include Common.h for the configuration, instead of erroring out. If there are circular dependencies we can split the configuration ifdef into a separate Core/Config.h. Maybe that's a good idea anyway if we want to separately include the configuration for some reason.

With those changes and the CMake configuration changes mentioned earlier, I'm fine with merging this :)

rokups · 2020-07-21T11:10:56Z

Oh but i am not suggesting to include lua when it is not used. We configure use of lua at build time, however. Thing is lua support is essentially a part of main library. I think original developer split it to a separate dll because intention was a commercial middleware, where you could ignore a part of unused functionality without having to recompile a library. And now that library is opensource this makes little sense. I mean look at Lua/Header.h and Core/Header.h. They are essentially same thing and only make sense when building library as bunch of dlls. Anyway this is starting to be quite a discussion of it's own so maybe lets indeed ignore it for now :)

I have dropped Common.h altogether. Instead inclusion of custom config via RMLUI_USER_CONFIG_FILE is moved to Config.h, then Types.h and Traits.h include Config.h, and we no longer need to massively modify includes of many files. Seems to me like now we have a good merge candidate.

mikke89 · 2020-07-21T11:27:22Z

Ah, I see what you mean now. Yeah that actually makes sense. This would make it harder to provide binaries though, but binaries for C++ libraries are problematic enough so I guess that's okay. But yeah let's ignore it for now and take that discussion later if we care enough to change :)

I have dropped Common.h altogether. Instead inclusion of custom config via RMLUI_USER_CONFIG_FILE is moved to Config.h, then Types.h and Traits.h include Config.h, and we no longer need to massively modify includes of many files. Seems to me like now we have a good merge candidate.

Yup, I like those changes. Thanks!

Looks good for merging to me. I might change up the CMake configuration a bit later.

mikke89 · 2020-07-21T12:01:11Z

Thanks again for the PR :)

rokups · 2020-07-21T12:18:40Z

Looks like i left an oopsie: https://github.com/mikke89/RmlUi/blob/master/Source/Core/StyleSheetParser.h#L42
Could you..? please..? 😊

mikke89 · 2020-07-21T12:26:39Z

Yup, I got it! :)

rokups force-pushed the alt-stl branch from eac8ced to ae7748d Compare July 12, 2020 07:13

mikke89 added the enhancement New feature or request label Jul 12, 2020

mikke89 added this to the 4.0 milestone Jul 12, 2020

mikke89 reviewed Jul 14, 2020

View reviewed changes

rokups force-pushed the alt-stl branch from 49f4c84 to f03d8c8 Compare July 14, 2020 16:08

mikke89 reviewed Jul 18, 2020

View reviewed changes

rokups force-pushed the alt-stl branch from 6c37fde to 40059df Compare July 19, 2020 06:37

mikke89 reviewed Jul 19, 2020

View reviewed changes

Include/RmlUi/UserConfig.h Outdated Show resolved Hide resolved

mikke89 reviewed Jul 19, 2020

View reviewed changes

Include/RmlUi/UserConfig.h Outdated Show resolved Hide resolved

mikke89 reviewed Jul 19, 2020

View reviewed changes

Include/RmlUi/Core/Types.h Outdated Show resolved Hide resolved

rokups force-pushed the alt-stl branch 2 times, most recently from 791dde2 to 7925763 Compare July 21, 2020 06:30

rokups and others added 4 commits July 21, 2020 09:38

Fix build with EASTL when it is configured to use size_type different…

21a7a5f

… from size_t.

Add ability for user to override container types.

c261c1f

Resolves mikke89#107.

Extend example of custom type conversion operators.

be4f774

Update Include/RmlUi/UserConfig.h

5a99538

Co-authored-by: Michael R. P. Ragazzon <mikke89@users.noreply.github.com> Update Include/RmlUi/Core/Types.h Co-authored-by: Michael R. P. Ragazzon <mikke89@users.noreply.github.com>

rokups force-pushed the alt-stl branch from 7925763 to 4bc85d8 Compare July 21, 2020 06:38

Move config file into a subdir and include it in Traits.h and Types.h

230cebe

rokups force-pushed the alt-stl branch from 4bc85d8 to 230cebe Compare July 21, 2020 11:09

mikke89 merged commit da48726 into mikke89:master Jul 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability for user to override container types #110

Add ability for user to override container types #110

rokups commented Jul 11, 2020

rokups commented Jul 12, 2020

mikke89 commented Jul 12, 2020

mikke89 left a comment

mikke89 Jul 13, 2020

mikke89 Jul 13, 2020 •

edited

Loading

mikke89 Jul 14, 2020

mikke89 Jul 14, 2020

mikke89 Jul 18, 2020 •

edited

Loading

rokups Jul 19, 2020 •

edited

Loading

mikke89 Jul 19, 2020

mikke89 commented Jul 19, 2020

rokups commented Jul 19, 2020

mikke89 commented Jul 19, 2020

rokups commented Jul 19, 2020

mikke89 commented Jul 19, 2020

rokups commented Jul 19, 2020 via email

mikke89 commented Jul 19, 2020

rokups commented Jul 21, 2020

mikke89 commented Jul 21, 2020

rokups commented Jul 21, 2020

mikke89 commented Jul 21, 2020

mikke89 commented Jul 21, 2020

rokups commented Jul 21, 2020

mikke89 commented Jul 21, 2020

Add ability for user to override container types #110

Add ability for user to override container types #110

Conversation

rokups commented Jul 11, 2020

rokups commented Jul 12, 2020

mikke89 commented Jul 12, 2020

mikke89 left a comment

Choose a reason for hiding this comment

mikke89 Jul 13, 2020

Choose a reason for hiding this comment

mikke89 Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

mikke89 Jul 14, 2020

Choose a reason for hiding this comment

mikke89 Jul 14, 2020

Choose a reason for hiding this comment

mikke89 Jul 18, 2020 • edited Loading

Choose a reason for hiding this comment

rokups Jul 19, 2020 • edited Loading

Choose a reason for hiding this comment

mikke89 Jul 19, 2020

Choose a reason for hiding this comment

mikke89 commented Jul 19, 2020

rokups commented Jul 19, 2020

mikke89 commented Jul 19, 2020

rokups commented Jul 19, 2020

mikke89 commented Jul 19, 2020

rokups commented Jul 19, 2020 via email

mikke89 commented Jul 19, 2020

rokups commented Jul 21, 2020

mikke89 commented Jul 21, 2020

rokups commented Jul 21, 2020

mikke89 commented Jul 21, 2020

mikke89 commented Jul 21, 2020

rokups commented Jul 21, 2020

mikke89 commented Jul 21, 2020

mikke89 Jul 13, 2020 •

edited

Loading

mikke89 Jul 18, 2020 •

edited

Loading

rokups Jul 19, 2020 •

edited

Loading