HomeWildfire Games

Improve UTF-8 character handling in Atlas
AuditedrP22335

Tags
None
Subscribers
Tokens
"Love" token, awarded by Itms."Like" token, awarded by elexis.

Description

Improve UTF-8 character handling in Atlas

(Also prevents the compile-time warnings reported in the abandoned D1432)

Accepted by: Itms
Patch linting by: Stan, Vladislav, wraitii
Also tested by: Imarok
Fixes: #4936
Differential Revision: https://code.wildfiregames.com/D1395

Event Timeline

Itms awarded a token.Jun 4 2019, 11:12 AM
elexis raised a concern with this commit.Jul 5 2019, 6:21 PM
elexis added a subscriber: elexis.
elexis added inline comments.
/ps/trunk/source/tools/atlas/GameInterface/ActorViewer.h
38

Compiling without precompiled headers:

../../../source/tools/atlas/GameInterface/ActorViewer.h:38:39: error: ‘CStr’ does not name a type
  38 |  void SetActor(const CStrW& id, const CStr& animation, player_id_t playerID);

Forward declaration of CStr8 fixes

This commit now has outstanding concerns.Jul 5 2019, 6:21 PM
elexis removed an auditor: elexis.Jul 7 2019, 11:55 PM
This commit no longer requires audit.Jul 7 2019, 11:55 PM
Stan added a subscriber: Stan.Jul 18 2019, 2:46 PM
Stan added inline comments.
/ps/trunk/source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
175

MSVC2015: warning C4130: '==': logical operation on address of string constant

Maybe use http://www.cplusplus.com/reference/string/string/compare/

or

/http://www.cplusplus.com/reference/cstring/strcmp

178

Same here.

s0600204 added inline comments.Jul 29 2019, 1:13 AM
/ps/trunk/source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
175

@Stan, does the following work for you? (If so, I'll commit the change):

diff --git a/source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp b/source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
index 85e2edee8..8126c2710 100644
--- a/source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
+++ b/source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
@@ -172,10 +172,10 @@ static AtObj ConvertToLatestFormat(AtObj in)
                AtObj actor;
 
                // Handle the global actor properties
-               if (in["Object"]["Properties"]["@autoflatten"] == "1")
+               if (strcmp(in["Object"]["Properties"]["@autoflatten"], "1") == 0)
                        actor.add("autoflatten", "");
 
-               if (in["Object"]["Properties"]["@castshadows"] == "1")
+               if (strcmp(in["Object"]["Properties"]["@castshadows"], "1") == 0)
                        actor.add("castshadow", "");
 
                // Things to strip leading strings (for converting filenames, since the
elexis added inline comments.Aug 3 2019, 1:06 PM
/ps/trunk/source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
175

Just because that removes the syntax compiler warning doesn't mean that the semantics are right.

AtIter is defined in AtlasObject.h.

[@autoflatten] will // Return an iterator to the children matching 'key'. (That is, children of the AtObj currently pointed to by this iterator)

I suppose this operator is called implicitly by the "1" conversion?

// Return the string value of the AtObj currently pointed to by this iterator
operator const char* () const;

Then I suppose the code is correct, but I suppose those arent the only two lines where one wants to perform this comparison, so perhaps it would be cleaner to make it so that people don't run into this again.

The other code appears a bit more typesafe:

version = (in["actor"]["@version"].defined()) ? (*in["actor"]["@version"]).getLong() : 0;

Some .getString might be nice, or even storing string types instead of char pointers.

Either way, I suggest to test the code to be actually functional.

I can't see any occurrence of autoflatten in the code, it seems to have been removed.

castshadows still there.

Clicking the "shadows" button the Atlas editor doesnt do anything for me, with or without strcmp.
(In svn shadows are always displayed, in a23 they are never displayed in actor viewer for me)

(I stop investigation here for now)

Stan added inline comments.Aug 18 2019, 7:01 PM
/ps/trunk/source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
175

This code is dead code. It used to work back when actors had properties tags see rP1762
nuked in rP2763 from all actors

It seems to be backwards compatibility for -1 0 versions of actors.

elexis raised a concern with this commit.Aug 29 2019, 2:55 AM

This breaks animations in the ActorViewer (as in not playing), but only on windows: #5574.

This commit now has outstanding concerns.Aug 29 2019, 2:55 AM
elexis added a comment.Sep 8 2019, 2:25 PM

So it seems the animations don't play because the autobuild doesn't commit the Atlas dll!
Which means only the compile warning would be left over if we can confirm it to be working after that fix (#5574 / D2021).

Stan accepted this commit.Mar 15 2020, 5:15 PM

Warning was fixed in rP23531

All concerns with this commit have now been addressed.Jul 27 2020, 10:58 AM