Migrate from libRocket to RmlUi #1715

slipher · 2022-01-17T03:25:25Z

Note that this is based on a March 2020 version of RmlUi, so there is still a bit of migration to get up to the latest version.

Companion UnvanquishedAssets/unvanquished_src.dpkdir#95

Fixes #1200.
Fixes #1551.

TODO: consider squashing commits up to the point where it at least compiles and runs without crashing.

Edit by illwieckz.

Needs:

Enable C++14 for RmlUi DaemonEngine/Daemon#572

ghost · 2022-01-18T13:33:47Z

I do not know how CI passes, but here I can't compile at all, something unsets the -std=c++14 that I put in the CXXFLAGS, somewhere, somehow, and since this is required for RMLUI, it does not builds.

Also, I think the review process could be easier if a pre-commit would do use namespace Rocket;, so that the 1st rmlUI commit would simply switch those to use namespace rml; and reduce a lot the number of changes to view.
I stopped reviewing around line 1400 of the 1st commit's diff, in case you'd be ok to do that (or if you prefer someone else to do it, I can, because I really think this would ease things).

slipher · 2022-01-18T17:31:52Z

You can check out the rmlui3/sync branch on Daemon to get C++14 activated.

The 1st commit I believe just contains the trivial find-and-replace stuff like Rocket -> Rml and CString -> c_str. I guess we have to replace that stuff at some point. I want it to be reviewable but I don't understand how putting it in a later commit would help, can you explain more?

ghost · 2022-01-18T17:41:07Z

The 1st commit I believe just contains the trivial find-and-replace stuff like Rocket -> Rml and CString -> c_str.

Nope, there's more than just that, I've seen stuff removed, notably.

I want it to be reviewable but I don't understand how putting it in a later commit would help, can you explain more?

Well, when the code have things like: Rocket::Core::String... which are replaced by rml::Core::String..., tha's one diff for each change.
If the code was using using namespace Rocket; before, then there would only be Core::String... which would remove all those changes from the diff.
Of course, those changes would have to be in a rocket/rml unrelated commit before, but it would bee much more trivial to review since the only diff in there would be either the removal of the Rocket:: string or the addition of the using namespace Rocket; at start of file.

slipher · 2022-01-19T10:36:59Z

@bmorel I've moved the most common and trivial renaming parts into a separate commit (the 2nd one).

The 1st commit now consists of the original first 20 or so commits squashed together. The minimal stuff to get it to build and launch (though mostly without displaying anything). I used a define hack to delay the renaming stuff.

slipher · 2022-01-21T01:18:48Z

Just realized this contradicts our compatibility guidelines by making it impossible to use master gamelogic with the latest release's DPKs. Thoughts anyone? Should it go on the 0.53 branch?

illwieckz · 2022-01-21T21:28:24Z

It's fine to go on 0.53.

ghost · 2022-01-22T10:54:47Z

How does it conflicts? The unvanquished.dpk also contains the GUI configuration files. If the RML changes are to be considered breaking compat, then some of the changes I made to bots also are, since I changed interface of BotGetHealScore (and probably other stuff) and the BTs are not in the nexe.
It is possible to have a package built from only sgame or only cgame, without the data, and to rely on unvanquished dpk to provide those, but that's not how people do it so far (except me, and that's a very recent change, done to make it faster for all to upload/download only the data or only the binary patches), and doing things in a different way than upstream should imply some understanding on the consequences (in this case, to provide the GUI file at the correct format).

What is likely required though, is to have this PR accompanied with the .rml files themselves, which are in a submodule (see #1714 about ending the split of inter-dependent files into 2 different repos, without much answers so far) when necessary (I do not know how much rml and rocket are compatible data-wise).

slipher · 2022-01-22T20:34:25Z

I want master gamelogic to be in a more-or-less usable state without any -pakpath flags, if you have the latest release installed. The bot changes were small enough that they still mostly work with 0.52 behavior trees. Similarly the yellow build shader is broken without new assets, but it's a minor point that doesn't prevent testing most things.

DolceTriade · 2022-01-25T21:07:07Z

Thanks for taking this over!

DolceTriade · 2022-01-25T21:20:47Z

It builds for me. I'll go test it some more later tonight.

ghost

Only reviewed the 1st commit, which LGTM despite some very minor details.
I will continue reviewing the other commits later.

ghost · 2022-02-16T00:25:04Z

src/cgame/rocket/rocket.cpp

@@ -196,34 +196,34 @@ class DaemonRenderInterface : public Rocket::Core::RenderInterface
 public:
 	DaemonRenderInterface() { }

-	void RenderGeometry( Rocket::Core::Vertex *verticies,  int numVerticies, int *indices, int numIndicies, Rocket::Core::TextureHandle texture, const Rocket::Core::Vector2f& translation )
+	void RenderGeometry( Rocket::Core::Vertex *verticies,  int numVerticies, int *indices, int numIndicies, Rocket::Core::TextureHandle texture, const Rocket::Core::Vector2f& translation ) override


I think those changes (the override thing) could already be merged. Not that important though, let's not waste time on such details.

ghost · 2022-02-16T00:25:30Z

src/cgame/rocket/rocket.cpp

+
+	void SetTransform( const Rocket::Core::Matrix4f* transform ) override
+	{
+		// TODO: implement.


maybe use and ASSERT( false ) in here? Just in case nobody ever fix that TODO?

I already have a Log::Warn

My comment was done when reading 1st commit, the warn was added in "RMLUI: warn if it calls unimplemented transform"

src/cgame/rocket/rocketChatField.h

ghost · 2022-02-16T00:42:51Z

src/cgame/rocket/rocketChatField.h

@@ -317,35 +320,31 @@ class RocketChatField : public Rocket::Core::Element, Rocket::Core::EventListene

 	void UpdateCursorPosition()
 	{
-		if ( text_element->GetFontFaceHandle() == nullptr )
+		if ( text_element->GetFontFaceHandle() == 0 )


from their code: RmlUi/Core/Types.h:using FontFaceHandle = uintptr_t; GetFontFaceHandle() returns a pointer, so let's use a nullptr instead of 0?

uintptr_t is a kind of integer

This is what I see:

- if ( text_element->GetFontFaceHandle() == nullptr ) + if ( text_element->GetFontFaceHandle() == 0 )

You mean that 0, NULL are integers? That is technically true (even if this comment seems to disagree for some cases), but I don't see the point to change from a semantically clearer version to a semantically less clear version. To use 6 less characters?

0 is an integer, and if you have using FontFaceHandle = uintptr_t;, then you should test for equality against the same type like slipher did, so ==0.

ghost · 2022-02-16T00:58:17Z

src/cgame/rocket/rocketElement.h

@@ -53,7 +53,7 @@ class RocketElement : public Rocket::Core::Element
 	RocketElement( const Rocket::Core::String &tag ) : Rocket::Core::Element( tag ) { }
 	~RocketElement() { }

-	bool GetIntrinsicDimensions( Rocket::Core::Vector2f &dimension )
+	bool GetIntrinsicDimensions( Rocket::Core::Vector2f &dimension ) override


see previous comment about override (I won't annotate the other ones)

ghost · 2022-02-16T01:04:46Z

src/cgame/rocket/rocketProgressBar.h

 		}

-		if ( changed_properties.find( "image" ) != changed_properties.end() )


just checking that this is not accidental change and that I understand (could be useful some day): image is no longer a property, but an attribute?

I guess the stuff you're talking about must have been deleted in the later commit which changes the class to inherit from ElementProgressBar

Indeed, I didn't reviewed the squashed PR but each commits one by one

ghost · 2022-02-16T01:12:21Z

src/cgame/rocket/rocket_documents.cpp

-
-		document->RemoveReference();
-	}
+	// TODO: Remove.


I guess this TODO is done in later commits? Can't really assert anything here, but maybe it would be possible to use deprecated attribute to avoid forgetting (would have a compiler warning, which can be checked or turned as error)?
Since C++14 is required for RMLUI anyway, let's use it?

ghost · 2022-02-16T01:18:30Z

src/cgame/rocket/rocket.cpp

@@ -679,7 +679,8 @@ class EngineCursor
        }
        else if ( show_cursor )
        {
-            mode = MouseMode::CustomCursor;


maybe get some Log::Notice for this?

ghost · 2022-02-16T01:25:06Z

src/cgame/rocket/rocketKeyBinder.h

@@ -55,7 +55,7 @@ class RocketKeyBinder : public Rml::Core::Element, public Rml::Core::EventListen
 	{
 	}

-	void OnAttributeChange( const Rml::Core::ElementAttributes &changed_attributes )
+	void OnAttributeChange( const Rml::Core::ElementAttributes &changed_attributes ) override


Most of those changes could have been done in the rocket code (would have fixed a bunch of warnings, too, and possibly could enable treating the lack of override as error).

Note that I was referring to the override changes, in the commit they were introduced.

ghost · 2022-02-16T01:30:57Z

src/cgame/cg_rocket.cpp

@@ -99,13 +99,7 @@ void CG_Rocket_Init( glconfig_t gl )
 		{
 			token = COM_Parse2( &text_p );

-			rocketInfo.cursor = trap_R_RegisterShader( token, (RegisterShaderFlags_t) RSF_DEFAULT );


This looks like it could be squashed with 18adf9a ?

ghost · 2022-02-16T01:31:32Z

src/cgame/cg_rocket.cpp

+	// Scale cursor with resolution while maintaining the original aspect ratio.
+	int x, y;
+	trap_R_GetTextureSize( rocketInfo.cursor, &x, &y );
+	float ratio = static_cast<float>( x ) / static_cast<float>( y );


This reminds me a TODO I kept in another PR :) a single cast would suffice (not important at all though).

src/cgame/cg_rocket.cpp

ghost · 2022-02-16T08:32:32Z

src/cgame/rocket/rocketProgressBar.h

-			}
-		}
-	}
+	virtual ~RocketProgressBar() {}


override?
Also, there is the = default thing. It's a bit longer, though, but makes it easy to notice with syntax coloration. Dunno about it's use.

I meant, since the class inherits, the DTor could use override instead.

src/cgame/rocket/rocket.h

ghost · 2022-02-16T09:01:41Z

src/cgame/rocket/rocket_documents.cpp

+// Scale the UI proportional to the screen size
+void Rocket_SetDocumentScale( Rml::Core::ElementDocument& document )
+{
+	// This gets 12pt on 1920�1080 screen, which is libRocket default for 1em


Nitpick: not sure it's worth using the UTF8 character for × here, looks like just asking for problems to me (even if should not do problems to most).

It's just a comment, so what's the worst that could happen?

ghost · 2022-02-16T09:05:18Z

src/cgame/cg_rocket_draw.cpp

@@ -2130,17 +2130,20 @@ class PredictedMineEfficiencyElement : public HudElement
 			{
 				if        ( deltaBudget < 0 ) {
 					color = Color::Red;
-					msg = va( "<span class='material-icon error'>&#xE000;</span> You are losing build points!"
+					// Icon U+E000


I think I've seen in other commits things to convert to/from UTF-8, maybe it would be possible to use them instead of manually made UTF-8 codes? (I might be wrong though)

These are Private Use Area characters, not displayable by normal fonts

Ok then, but I would not expect anyone to be able to guess that. Is there something to do to help on that? (adding comment everywhere might be counter-productive, if this happens in other places, but I do not think I've seen other ones?)

ghost · 2022-02-16T09:11:20Z

src/cgame/cg_rocket_draw.cpp

 	rocketElementType_t type;
 };

+// This kind of element is a hack left over from when VMs could only use C code


just asking, maybe this could use the deprecate thingy of C++14? Or a TODO, since this looks like a pretty good candidate of things to get rid of soon (those manually sorted list are quite annoying, but at least here it's documented...)

I don't see how, since deprecation warnings would fire for all uses, not only new ones.

True, but it might help encourage us to fix the situation if we see them :)

ghost · 2022-02-16T09:16:01Z

src/cgame/rocket/rocket.cpp

@@ -140,6 +140,14 @@ class DaemonSystemInterface : public Rml::Core::SystemInterface

 	void SetMouseCursor(const Rml::Core::String& cursor_name) override
 	{
+		if ( cursor_name.empty() )


Maybe it's somehow possible to prevent the cursor's name to get empty, instead?
Also, this commit looks like it could be squashed with e61b3a3 ?

The cursor name becomes empty when there is no document in menuContext under the cursor's position. I guess that could be prevented by drawing an invisible document over the whole screen, but that hardly seems nicer.

This clearly would feel like dirty hack. Maybe in later rmlui version it's possible to get a default cursor... we'll see I guess.

src/cgame/rocket/rocket.cpp

ghost · 2022-02-16T09:34:41Z

I have reviewed each commits one by one, still have to read the companion PR though.

Overall things seems ok, but I really think the override stuff could go in separate commit which would apply to current state of code. This is not important though, as long as we get rid of those... also, this means we could be able to enable the related warnings (including in CI), which is a good thing.
Some commits could likely be squashed to make reviewing easier, too, but in my case it's too late. Of course, the PR needs a merge so I guess it will have to be reviewed again...

The thing I would like to see changed the most is the addition of a macro where a template could apply. Since templates are instantiated once per type and since there's exactly one instance of each instanciers, this would work. What would probably be needed in my patch is some added documentation in the template or even better, some kind of actual security, no idea how (verifying that the function is called exactly once, maybe?).

sed -b -i -E 's/\bRocket::/Rml::/g; s/namespace Rocket/namespace Rml/; s/CString/c_str/g; s/\.Append\b/.append/; s/\.Empty\b/.empty/g; s/\.Length\b/.size/; s/PropertyNameList/PropertyIdSet/; ' $(find src -type f)

TODO: Revisit this in the future.

In the future, we should dynamically load cursors, but that's TODO.

This way, they will always be received by the event listener.

This allows individual menus to have custom cursors if they want.

Use this instead of our custom one for our progress bar.

In cg_rocket_draw.cpp, we would create a new instancer, and never delete it. Instead, we store it in a static unique_ptr<> so it will be cleaned up when the program terminates or the next time rocket is initialized. Also, consolidate the RegisterElement logic.

RmlUi apparently recognizes the border-width shorthand unlike librocket, so rename it to momentum-border-width to avoid conflicts.

Apparently HTML entities were removed from RmlUi, so I had to replace them with the UTF-8 byte sequences.

In RmlUi modifying the element in OnRender no longer works well: if you change the element text in OnRender, then the element becomes blank for one frame, causing flickering.

Also fix the calculation for how many lines can be shown. This was broken before RMLUI and it was stuck at the fallback value of 4 lines.

This is used for example if you type Chinese characters in a chat message, since they are not in Roboto.

Instead of intercepting events at the root element, I've added a Lua snippet to the RML to focus the chatfield element when it is opened. There are a lot of bugs with the chat field, but I think I've squashed all of the ones that are new with RmlUi.

In particular it happened when the console is closed on the main menu. To prevent the bug from recurring in another case, when the console is open and the mouse is outside the window after the vid_restart, a commit to remove the duplicate call to IN_Init in Daemon is needed.

Co-authored-by: Morel Bérenger <berengermorel76@gmail.com>

illwieckz · 2022-02-19T12:43:51Z

The first commit of the branch had a wrong submodule because of UnvanquishedAssets/unvanquished_src.dpkdir#95 (comment)

If you have pulled Unvanquished 0.53.0/sync, you have to reset to b8dd98d and pull again.

slipher mentioned this pull request Jan 17, 2022

Migrate from libRocket to RmlUi UnvanquishedAssets/unvanquished_src.dpkdir#95

Merged

slipher force-pushed the rmlui3/sync branch from 754c9a5 to 4302c05 Compare January 17, 2022 18:32

slipher force-pushed the rmlui3/sync branch from 4302c05 to 51bc4cd Compare January 19, 2022 10:32

slipher force-pushed the rmlui3/sync branch from 51bc4cd to 9dcd9cf Compare January 19, 2022 22:22

slipher changed the base branch from master to 0.53.0/sync January 22, 2022 20:34

slipher force-pushed the rmlui3/sync branch from 9dcd9cf to b6b0c6e Compare January 22, 2022 20:39

ghost reviewed Feb 16, 2022

View reviewed changes

src/cgame/cg_rocket.cpp Show resolved Hide resolved

ghost reviewed Feb 16, 2022

View reviewed changes

src/cgame/rocket/rocket.h Outdated Show resolved Hide resolved

ghost reviewed Feb 16, 2022

View reviewed changes

src/cgame/rocket/rocket.cpp Outdated Show resolved Hide resolved

slipher and others added 26 commits February 19, 2022 00:27

RMLUI: migrate trivial string replacements

290861e

sed -b -i -E 's/\bRocket::/Rml::/g; s/namespace Rocket/namespace Rml/; s/CString/c_str/g; s/\.Append\b/.append/; s/\.Empty\b/.empty/g; s/\.Length\b/.size/; s/PropertyNameList/PropertyIdSet/; ' $(find src -type f)

RMLUI: RmlUi declares font-size now, so don't declare it ourselves.

ac7b5c5

RMLUI: add hack to render a system cursor for RmlUi

4ac8a9e

TODO: Revisit this in the future.

RMLUI: fix rocketColorInput focus issues

f712ccf

RMLUI: fix up rocketFormControlInput to correctly set cvars

65c97b6

RMLUI: fix rocketKeyBinder

c0d33ec

RMLUI: hardcode custom cursors in the rocket.txt for now.

22d0876

In the future, we should dynamically load cursors, but that's TODO.

RMLUI: add rocketFocusManager's events in the capture phase.

193ee46

This way, they will always be received by the event listener.

RMLUI: implement mouse cursors utilizing rcss properly

616fe11

This allows individual menus to have custom cursors if they want.

RMLUI: Use the builtin RmlUi progressbar

6f573ab

Use this instead of our custom one for our progress bar.

RMLUI: re-implement UI scaling to screen size

a5932e4

RMLUI: rename custom RCSS property border-width

18335ea

RmlUi apparently recognizes the border-width shorthand unlike librocket, so rename it to momentum-border-width to avoid conflicts.

RMLUI: fix Material Icons glyphs

ee9c8d1

Apparently HTML entities were removed from RmlUi, so I had to replace them with the UTF-8 byte sequences.

RMLUI: fix miner placement text not showing

eb2ecfc

RMLUI: set RML in OnUpdate, not OnRender

843c8e5

In RmlUi modifying the element in OnRender no longer works well: if you change the element text in OnRender, then the element becomes blank for one frame, causing flickering.

RMLUI: warn if it calls unimplemented transform

0dff213

RMLUI: fix cursor disappearing outside menu

cc21716

RMLUI: fix chat/console lines not being shown

12fe033

Also fix the calculation for how many lines can be shown. This was broken before RMLUI and it was stuck at the fallback value of 4 lines.

RMLUI: restore unifont as backup font

50733c4

This is used for example if you type Chinese characters in a chat message, since they are not in Roboto.

RMLUI: fix cursor not showing on main menu

c5a8845

RMLUI: fix sliders not updating their value

1a49676

Mark functions override in cg_rocket_draw.cpp

4ab8048

Rewrite REGISTER_ELEMENT for RmlUi as non-macro

d002e51

Co-authored-by: Morel Bérenger <berengermorel76@gmail.com>

slipher force-pushed the rmlui3/sync branch from 4f3884b to d002e51 Compare February 19, 2022 06:29

slipher merged commit 212ecdb into 0.53.0/sync Feb 19, 2022

slipher deleted the rmlui3/sync branch February 19, 2022 06:55

		}

		if ( changed_properties.find( "image" ) != changed_properties.end() )

Migrate from libRocket to RmlUi #1715

Migrate from libRocket to RmlUi #1715

Conversation

slipher commented Jan 17, 2022 • edited by illwieckz

ghost commented Jan 18, 2022 • edited by ghost

slipher commented Jan 18, 2022

ghost commented Jan 18, 2022 • edited by ghost

slipher commented Jan 19, 2022

slipher commented Jan 21, 2022

illwieckz commented Jan 21, 2022

ghost commented Jan 22, 2022

slipher commented Jan 22, 2022

DolceTriade commented Jan 25, 2022

DolceTriade commented Jan 25, 2022

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Feb 17, 2022 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Feb 17, 2022 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 16, 2022

illwieckz commented Feb 19, 2022

slipher commented Jan 17, 2022 •

edited by illwieckz

ghost commented Jan 18, 2022 •

edited by ghost

ghost commented Jan 18, 2022 •

edited by ghost

ghost Feb 17, 2022 •

edited by ghost

ghost Feb 17, 2022 •

edited by ghost