Page MenuHomeWildfire Games

JSInterface getTextSize only for TextOwner
ClosedPublic

Authored by elexis on Jul 30 2019, 5:48 PM.

Details

Summary

As mentioned in rP22134 / D844 / #5442, the getTextSize function has some issues:

  • It can only work for few GUI Types (those with a caption string), but the function is exposed to all GUI types.
  • It is a copypaste of what happens in the users of GUITextOwner (most notably CText) without this being apparent unless happening to look at both places (so likely that futuer improvements to one of the copies will not be applied to the other copies, and it's more to maintain).
  • The CSize conversion should use ToJSVal / FromJSVal to avoid redundancy and improve abstraction / shorten.

So moving the function to GUITextOwner removes the code duplication, removes redundant DrawText calls and improves encapsulation / separation of concernis.

Test Plan

Check whether the dynamic_cast is sane. Confirm that the computed sizes are the same as before by comparing against what CText does and warn(uneval())ing sample calls. Modify the viewer.xml size in order to see whether the buffer and scrollbar size are accounted for equally.
Possibly compare against D1781 which also moves some JSInterface_IGUIObject code to a new JSInterface file in accordance with the trac ticket #5442.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jul 30 2019, 5:48 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/gui/IGUITextOwner.h
| 168| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/scripting/JSInterface_IGUITextOwner.h
|  23| namespace·JSI_IGUITextOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceJSI_IGUITextOwner{' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIObject.h
| 168| »   ·*
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/272/display/redirect

Adding the virtual void* GetTextOwner() workaround invented add D1781 that is broken by design because the base class should be agnostic of the derived class, but has the advantage of avoiding the otherwise necessary dynamic_cast, under the reservation that there are only three virtually inheriting classes in source/, this being one of them.

In D2136#89216, @elexis wrote:

Adding the virtual void* GetTextOwner() workaround invented add D1781 that is broken by design because the base class should be agnostic of the derived class, but has the advantage of avoiding the otherwise necessary dynamic_cast, under the reservation that there are only three virtually inheriting classes in source/, this being one of them.

TBH I think we might be running in an unsolvable problem here so long as we keep using hierarchies, and perhaps instead we should try moving towards composition? Would be a larger refactoring of course.


The annoying thing is that this wouldn't be needed if these weren't free functions...

I tested the dynamic_cast performance using:

	{
		auto start = std::chrono::system_clock::now();
		obj = static_cast<IGUITextOwner*>(ob->GetTextOwner());
		auto end = std::chrono::system_clock::now();
		debug_printf("GetTextOwner() took %ld\n", std::chrono::duration_cast<std::chrono::nanoseconds>(end - start).count());
	}

	{
		auto start = std::chrono::system_clock::now();
		obj = dynamic_cast<IGUITextOwner*>(ob);
		auto end = std::chrono::system_clock::now();
		debug_printf("dynamic_cast took %ld\n", std::chrono::duration_cast<std::chrono::nanoseconds>(end - start).count());
	}

GetTextOwner() took 331
dynamic_cast took 16314

So as far as I see dynamic_cast is the only implementation that isn't broken, but its slow, so I take the broken workaround.

See also several hours of IRC discussions last night (or better don't see since it wasnt fruitful).

This revision was not accepted when it landed; it landed in state Needs Review.Aug 2 2019, 6:55 PM
This revision was automatically updated to reflect the committed changes.
elexis added a comment.Aug 2 2019, 6:59 PM

See discussion with Vladislav on IRC about the need to avoid dynamic_cast and an idea he had to change it that was accepted to be pursue independent of this diff.


http://irclogs.wildfiregames.com/2019-07/2019-07-31-QuakeNet-%230ad-dev.log

Since this requires rewriting all IGUIObject types and or all JSInterfaces, it may not be mixed with this diff.