Page MenuHomeWildfire Games

Handle UTF-8 multibyte characters in Atlas when loading from JSON
ClosedPublic

Authored by s0600204 on Mar 16 2018, 5:10 PM.

Details

Summary

UTF-8

The unicode character-code for ë is U+00EB.

The utf-8 specification states that the maximum value of a single-byte utf-8 character is 0x7F (0111 1111).

0xEB is greater, so in order to encode ë in utf-8, it is encoded as a multi-byte character, specifically 0xC3AB.

JSON loading/parsing

We load JSON files into a string of the std::string type. This stringified JSON then gets passed to a parser to be decanted into an actual object with keys and values. In main 0ad, this is handled by spidermonkey.

In Atlas, it is handled by the function ConvertNode(^1) feeding it though the third party jsonspirit(^2) library. When this function encounters a node that contains a string value, it runs the following line to store the value returned from jsonspirit in an AtObj:

obj->value = std::wstring(node.get_str().begin(), node.get_str().end());

node.get_str() returns a std::string. However, obj->value requires a std::wstring. Hence the conversion.

However, the conversion is done naively, and without respect for the rules of utf-8. From what I can tell, each char in node.get_str() gets converted to a wchar_t. This works fine for single-byte utf-8 characters. However when it comes to multi-byte utf-8 characters, all the separate bytes of a character should go into the same wchar_t - but that doesn't happen, instead being put into separate (albeit consecutive) wchar_ts.

Solution

The solution below rewrites AtObj to store strings as std::string instead of std::wstring.

(Historical note: The solution originally used in this revision was to use wxString as an intermediary to convert from std::string to std::wstring in a utf-8 multi-byte aware manner. This was ultimately considered a flawed approach.)


^1 - source/tools/atlas/AtlasObject/AtlasObjectJS.cpp line 44
^2 - https://www.codeproject.com/KB/recipes/JSON_Spirit.aspx

Test Plan

Note: It is imperative that this be tested on Linux, OSX and Windows, due to potential differing character handling specific to those systems.

Test the following both with and without the patch applied:

  • Open Atlas from your OS's console/terminal(-emulator)
    • (Press continue if necessary when testing without the patch applied)
  • Look at the list of Random Maps in the select/dropdown
  • Confirm that, with the patch applied, "Fields of Meroë" appears in the select correctly
  • Select and generate the map
    • If you're running without the patch, it will be the empty line between "Extinct Volcano" and "Flood"
  • Confirm that, with the patch applied, Generating Fields of Meroë of size [...] appears in the console with no corruption on/around the ë
  • Return to the Atlas window and confirm that the "Map Name" and "Description" text fields are auto-filled and have all their characters appearing correctly.

Please report if it works, and on what systems you've tested on.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Dec 27 2018, 7:11 PM
source/tools/atlas/AtlasObject/AtlasObjectJS.cpp
52 ↗(On Diff #6186)

Source code from pyrogenesis is not supposed to be used in AtlasUI projects (on Windows, AtlasUI gets built as a separate DLL which is standalone from the engine). So using this function is not possible. One could try to copy some code to the Atlas source but using wxString sounds perfectly good to me!

Copying the utf8 code sounds bad, because that's exactly what brought this issue, D1432, possibly more UTF conversion issues about (utf8.h getting fixes that are not applied to atlas).

Is it absolutely impossible to move that utf8 code out of pyrogenesis to any commonplace to avoid such duplication consequences (and maybe or maybe not end up with a nicer diff)?

Why are UTF8 strings preferable over UTF16 ones? Might be, but I didn't see the reason mentioned, and

  1. usually data is kept in the way the original data is provided (I failed to find the place where the strings are constructed)
  2. most of the changed lines use wstring

Also as mentioned in Phab:D1432#67442, the atlas utf code has been an exact copy of the source/lib/utf8.h code.

So we deal with an outdated copy that didn't receive bugfixes that the original copy received (as Vladislav mentioned).

(I don't know if one can easily get away with using wx string functions and never converting anything in the places that can't access wx;
but one might consider making lib/utf8.h accessible to atlas.)

Itms added a comment.Dec 27 2018, 7:48 PM
In D1395#67451, @elexis wrote:

Why are UTF8 strings preferable over UTF16 ones? Might be, but I didn't see the reason mentioned, and

  1. usually data is kept in the way the original data is provided (I failed to find the place where the strings are constructed)
  2. most of the changed lines use wstring

UTF8 is the format we use in our data files (json, xml). The strings are not really constructed but loaded raw from the files.
As for using UTF8 vs UTF16 in the files, there are pros and cons, but using UTF8 has the advantage of not masking the places where characters outside of the basic plane are not properly handled by our code.

Our code used wstring because wxwidgets uses UTF16 internally (I suppose that was the reason, at least). But wxwidgets allows us to work with UTF8 and provides FromUTF8 constructors when it's time for us to display text. We also have access to lib/utf8.h whenever we need to deal with the VFS (see the change line 422 of ObjectHandlers.cpp). So we really don't need to rack our brains to keep using UTF16 for handling data in Atlas.

(I don't know if one can easily get away with using wx string functions and never converting anything in the places that can't access wx;

That's exactly what this patch is doing, so it doesn't seem painful.

but one might consider making lib/utf8.h accessible to atlas.)

Be careful: one has to distinguish between the part of Atlas which is interfaced with the engine, in which we have access to that code; and the independant part of Atlas for which I have no idea how to make that code accessible, but fortunately we don't need to.

Itms added a comment.Dec 27 2018, 7:52 PM

@s0600204 This is a wonderful patch! 👍

I'm starting to test it and review it in depth (I didn't see any issue on a first read). I am going to check that removing the wxwidgets dependency of the AtlasObject project doesn't break anything. Did you try that already?

Stan added a subscriber: Stan.May 2 2019, 9:45 PM

@Itms any news on that review ? :P

Stan added inline comments.May 2 2019, 9:48 PM
source/tools/atlas/AtlasObject/AtlasObjectXML.cpp
64 ↗(On Diff #6609)

static_cast

85 ↗(On Diff #6609)

static_cast

111 ↗(On Diff #6609)

static_cast (reinterpret_cast maybe)

120 ↗(On Diff #6609)

Same as above.

s0600204 updated this revision to Diff 7938.May 8 2019, 7:20 PM
s0600204 marked 4 inline comments as done.

Rebase; and *_casts as requested by @Stan

source/tools/atlas/AtlasObject/AtlasObjectXML.cpp
64 ↗(On Diff #6609)

Couldn't get it to compile with static_cast, so went with reinterpret_cast. Perhaps that is a mistake - if so it might take someone with a better grasp of the c++ language to succeed.

Stan added inline comments.May 8 2019, 7:38 PM
source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp
59 ↗(On Diff #7938)

ternary ?

return m_Impl ? m_Impl->iter->second->m_Value.c_str() : ""
130 ↗(On Diff #7938)

Same here as above ?

145 ↗(On Diff #7938)

Any reason to keep this ?

source/tools/atlas/AtlasObject/AtlasObjectXML.cpp
64 ↗(On Diff #6609)

Nah last time I changed a cast to this type it happened to me too :) Default cast just tries everything until it works :)

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/Object.cpp
102 ↗(On Diff #7938)

Strange default value. but unrelated to this patch.

Stan added inline comments.May 8 2019, 7:43 PM
source/tools/atlas/AtlasObject/AtlasObjectXML.cpp
64 ↗(On Diff #7938)
Vulcan added a comment.May 8 2019, 7:44 PM

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

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 213| »   //·Iterate·over·variant·groups
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Messages.h
| 213| QUERY(VFSFileRealPath,
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/ActorViewer.cpp
| 213| »   void·UpdatePropListRecursive(CModelAbstract*·model);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
| 213| 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):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1346/display/redirect

s0600204 updated this revision to Diff 7997.May 13 2019, 6:25 PM
s0600204 marked 3 inline comments as done.

Code style updates (ternary)

Also actually do a thing I said I had. 🙄

s0600204 updated this revision to Diff 7998.May 13 2019, 6:29 PM

Here's a bright idea: let's upload the correct patch (duh!)

s0600204 added inline comments.May 13 2019, 6:32 PM
source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp
145 ↗(On Diff #7938)

Apart from it being used nine times?

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

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Messages.h
| 213| QUERY(VFSFileRealPath,
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/ActorViewer.cpp
| 213| »   void·UpdatePropListRecursive(CModelAbstract*·model);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
| 213| 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):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 213| »   //·Iterate·over·variant·groups
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1387/display/redirect

Itms requested changes to this revision.May 21 2019, 7:58 PM

Thanks again for the patch, now I finally have some time to dedicate to it!

The code looks good, here are a few extra inline comments.

I confirm that the wxWidgets dependency is now useless for AtlasObject 🎉 You can thus make the following changes:

Index: build/premake/premake5.lua
===================================================================
--- build/premake/premake5.lua	(revision 22292)
+++ build/premake/premake5.lua	(working copy)
@@ -1108,8 +1108,7 @@
 	},{	-- extern_libs
 		"boost",
 		"iconv",
-		"libxml2",
-		"wxwidgets"
+		"libxml2"
 	},{	-- extra_params
 		no_pch = 1
 	})
@@ -1356,7 +1355,6 @@
 	links { "mocks_test" }
 	if _OPTIONS["atlas"] then
 		links { "AtlasObject" }
-		project_add_extern_libs({"wxwidgets"}, target_type)
 	end
 	extra_params = {
 		extra_files = { "test_setup.cpp" },
Index: source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp
===================================================================
--- source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp	(revision 22292)
+++ source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp	(working copy)
@@ -21,8 +21,6 @@
 #include <assert.h>
 #include <sstream>
 
-#include <wx/string.h>
-
 #define ATSMARTPTR_IMPL(T) \
 	template<> void AtSmartPtr<T>::inc_ref()	\
 	{											\
Index: source/tools/atlas/AtlasObject/AtlasObjectJS.cpp
===================================================================
--- source/tools/atlas/AtlasObject/AtlasObjectJS.cpp	(revision 22292)
+++ source/tools/atlas/AtlasObject/AtlasObjectJS.cpp	(working copy)
@@ -24,8 +24,6 @@
 # pragma warning(disable:4996)	// deprecated CRT
 #endif
 
-#include "wx/log.h"
-
 #include <sstream>
 
 static AtSmartPtr<AtNode> ConvertNode(json_spirit::Value node);

Now unfortunately I have a bug in Atlas: if the name or description of a map contains special UTF-8 characters, they will not be saved. You can reproduce easily by just generating a map from Fields of Meroë, then saving it: in the saved XML file, the Name and Description fields will be empty. Removing the ë and saving again will work.

source/tools/atlas/AtlasObject/AtlasObjectText.h
29 ↗(On Diff #7998)

We could use the opportunity to remove this dead hack.

source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
202 ↗(On Diff #7998)

You can use boost::algorithm::starts_with like we already do in a few places of the code 🙂

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
308 ↗(On Diff #7998)

You can use a range-based for loop.

326 ↗(On Diff #7998)

Here as well.

This revision now requires changes to proceed.May 21 2019, 7:58 PM
s0600204 updated this revision to Diff 8105.Fri, May 24, 7:04 PM
s0600204 marked 4 inline comments as done.

Update in response to @Itms's comments.

Highlights:

  • Remove wxWidgets dependency from AtlasObject
  • Range-based instead of iterator-based fors
Owners added a subscriber: Restricted Owners Package.Fri, May 24, 7:04 PM
In D1395#78914, @Itms wrote:

Now unfortunately I have a bug in Atlas: if the name or description of a map contains special UTF-8 characters, they will not be saved. You can reproduce easily by just generating a map from Fields of Meroë, then saving it: in the saved XML file, the Name and Description fields will be empty. Removing the ë and saving again will work.

I am sorry, but I am unable to replicate this issue. (on Arch Linux)

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/ActorViewer.cpp
| 213| »   void·UpdatePropListRecursive(CModelAbstract*·model);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Messages.h
| 213| QUERY(VFSFileRealPath,
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
| 213| 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):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 213| »   //·Iterate·over·variant·groups
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1469/display/redirect

Itms requested changes to this revision.Sat, May 25, 11:52 AM

Sorry, my reproduction steps were not correct. What I actually tried was adding a char that is outside the first block (ξ, i.e. U+03BE). From my research, this doesn't fail in Atlas, but in CSimulation2::GetMapSettingsString(), so this is a SpiderMonkey bug, not related to your patch. It should be fixed later, I'll try to have a look as part of the SM update.

With the ë letter, loading and saving works, but I could still have a bug:

  1. Generate a map from Fields of Meroë
  2. Change the description (for instance vast ->vaste for a oh-so-posh French feeling 😉 )
  3. Save the map: the XML now contains a � instead of the ë.
source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
203 ↗(On Diff #8105)

We're unlikely to reach C++20 support anytime soon, and I guess we will replace all occurrences of this usage of boost when we do, so this todo is not really useful I think. I don't mind if you want to leave it, but if you do, we usually write them as // TODO: Foo. Keeping a consistent style is better for grepping for the todos.

This revision now requires changes to proceed.Sat, May 25, 11:52 AM
s0600204 updated this revision to Diff 8161.Sun, May 26, 5:25 PM

Resolve issue with reading above-ascii characters from Map Name and Description, and Player Name input fields.

Upon entering characters with a code-value above U+007F - eg. ë (U+00EB), ξ (U+03BE) - they should now be read from the input fields correctly.

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Messages.h
| 213| QUERY(VFSFileRealPath,
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 213| »   //·Iterate·over·variant·groups
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/ActorViewer.cpp
| 213| »   void·UpdatePropListRecursive(CModelAbstract*·model);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
| 213| 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):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1512/display/redirect

Itms accepted this revision.Tue, May 28, 7:16 PM

This works perfectly from me now, and the code still looks good 👍 I think the build error comes from the fact that the premake change makes the pch files invalid. I should improve Jenkins so that it performs cleanups when premake scripts are touched, but this is not trivial. I have an alternative fix in mind that I am implementing these days. Anyway, thanks for the great work on this!

This revision is now accepted and ready to land.Tue, May 28, 7:16 PM
vladislavbelov added inline comments.Tue, May 28, 7:40 PM
source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp
142 ↗(On Diff #8161)

It can be written as:

std::stringstream s(m_Node->m_Value);
ss >> val;

Others too.

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
308 ↗(On Diff #8161)

It's better to use const here:

for (const std::string& victoryCondition : m_MapSettingsVictoryConditions)
326 ↗(On Diff #8161)

The same here.

450 ↗(On Diff #8161)

(void*)(intptr_t)(*s["Tiles"]).getLong() => reinterpret_cast<void*>(s["Tiles"]->getLong()).

I think it can be replaced in other places too (where it's applicable): (*var).method() => var->method().

s0600204 updated this revision to Diff 8206.Wed, May 29, 8:35 PM
s0600204 marked 4 inline comments as done.

Update in response to @vladislavbelov's comments.

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
450 ↗(On Diff #8161)

(*var).method() => var->method().

gcc 8.3 says:

error: base operand of ‘->’ has non-pointer type ‘const AtIter’

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Messages.h
| 213| QUERY(VFSFileRealPath,
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/ActorViewer.cpp
| 213| »   void·UpdatePropListRecursive(CModelAbstract*·model);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
| 213| 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):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 213| »   //·Iterate·over·variant·groups
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1542/display/redirect

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
450 ↗(On Diff #8161)

Yeah, I forgot, it's our implemented iterator.

Stan added inline comments.Wed, May 29, 10:40 PM
source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
326 ↗(On Diff #8206)

Missing the & ?

308 ↗(On Diff #8161)

Missing the & ?

s0600204 updated this revision to Diff 8214.Thu, May 30, 5:52 PM
s0600204 marked 2 inline comments as done.

Add some ampersands (&) that @Stan noticed I'd missed.

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Messages.h
| 213| QUERY(VFSFileRealPath,
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 213| »   //·Iterate·over·variant·groups
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
| 213| 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):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/ActorViewer.cpp
| 213| »   void·UpdatePropListRecursive(CModelAbstract*·model);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1548/display/redirect

Stan added a comment.EditedThu, May 30, 6:15 PM

Some optional other comments. Don't mind the build it's broken.

source/tools/atlas/AtlasObject/AtlasObjectText.cpp
30 ↗(On Diff #8214)

if (!obj)

53 ↗(On Diff #8214)

Maybe so we don't assign first_child to false at every iteration

for (const AtNode::child_maptype::value_type& child : obj->m_Children)
{
    if (!first_child)
    {
        result += ", ";
        first_child = false;
    }

    result += ConvertRecursive(child.second);
}
source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
242 ↗(On Diff #8214)

const & ?

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/Object.cpp
547 ↗(On Diff #8214)

const & ?

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Player/Player.cpp
587 ↗(On Diff #8214)

Range loop ?

697 ↗(On Diff #8214)

See player["AI"].defined(); comment below;

709 ↗(On Diff #8214)

See player["AI"].defined(); comment below;

741 ↗(On Diff #8214)
		wxString aiID;
		defined = player["AI"].defined();
		if (defined)
			aiID = wxString::FromUTF8(player["AI"]);
		else
			aiID = wxString::FromUTF8(playerDefs["AI"]);

Could maybe turned into

wxString aiID = wxString::FromUTF8(player["AI"].defined() ? player["AI"], playerDefs["AI"]);
s0600204 updated this revision to Diff 8221.Thu, May 30, 8:21 PM
s0600204 marked 3 inline comments as done.

Update and clean-up.

I feel the scope of the suggestions is slipping into clean-up of surrounding code rather than critiquing the code alteration this revision was created to handle. Hence I've ignored some that I feel out-of-scope for this revision.

source/tools/atlas/AtlasObject/AtlasObjectText.cpp
53 ↗(On Diff #8214)

Stan, I'm going to let you think that through and realise why that wouldn't work. 🤔

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Player/Player.cpp
697 ↗(On Diff #8214)

In this case, there are three possible end states:

  • "Unknown"
  • player['Name']
  • playerDefs['Name']

I'm not a fan of nested ternaries, so I'm nope'ing this suggestion.

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/ActorViewer.cpp
| 213| »   void·UpdatePropListRecursive(CModelAbstract*·model);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
| 213| 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):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Messages.h
| 213| QUERY(VFSFileRealPath,
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 213| »   //·Iterate·over·variant·groups
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1552/display/redirect

Stan added inline comments.Thu, May 30, 8:53 PM
source/tools/atlas/AtlasObject/AtlasObjectText.cpp
53 ↗(On Diff #8214)

haha ^^

This revision was landed with ongoing or failed builds.Mon, Jun 3, 10:20 PM
This revision was automatically updated to reflect the committed changes.