Paths

Table of Contentst

Fix compiler warning (comparison against a string literal) in the ActorEditor following rP22335
ClosedPublic
Actions

Authored by Stan on Oct 27 2019, 4:37 PM.

Details

Reviewers

s0600204
Silier

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP23531: Fix warnings with VS2015 introduced in rP22335

Summary

As of rP22335 when compiling atlas on windows one gets a few warnings. Those two are easy to fix

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

At first I thought the code was useless and could be removed, but it's actually used to convert the old actor format (see attached example to the new format using the actor editor) into the new one using the actor editor. Not sure how much support we need to give for a 15 years old format, but it seems easy enough to fix.

fndn_4x4_old.xml412 BDownload

Without the current fix the castshadow property would not be present in the end generated actor

fndn_4x42_old.xml244 BDownload

With the patch:

fndn_4x42_old.xml260 BDownload

On gcc:

../../../source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp:175:50: warning: result of comparison against a string literal is unspecified
      (use strncmp instead) [-Wstring-compare]
                if (in["Object"]["Properties"]["@autoflatten"] == "1")
                                                               ^  ~~~
../../../source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp:178:50: warning: result of comparison against a string literal is unspecified
      (use strncmp instead) [-Wstring-compare]
                if (in["Object"]["Properties"]["@castshadows"] == "1")
                                                               ^  ~~~

Test Plan

Test that you can open a bunch of old actors. you can find them here: https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/official/art/actors?rev=2049&order=name
Test that there is no more warning in MSVC
Agree that it's the best way to fix it, as that's how it's done below.

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

Stan created this revision.Oct 27 2019, 4:37 PM

elexis edited the summary of this revision. (Show Details)Oct 27 2019, 4:40 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/490/display/redirect

Why does this check for defined instead of the value?
See the comments in rP22335 any autoflatten code looks like it should be deleted instead of modified.

elexis edited the summary of this revision. (Show Details)Oct 27 2019, 4:45 PM

elexis retitled this revision from Fix MSVC Warning and bad code revealed in rP22335 to Fix compiler warning (comparison against a string literal) in the ActorEditor following rP22335.

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

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

Good point it should actually read the value. I will nuke the autoflatten code.

Harbormaster completed remote builds in B9881: Diff 10211.Oct 27 2019, 4:46 PM

Remove autoflatten check whether the attribute it defined and whether it's an int == 1 (typing random text returns 0)

Use set instead of add.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/491/display/redirect

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

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

Harbormaster completed remote builds in B9883: Diff 10212.Oct 27 2019, 5:10 PM

So the question is what else should be deleted that is also relating to autoflatten (that's where I tuned out last time).
It would be good to get this over with since its one of the few (3?) compiler warnings remaining.

source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
175 ↗	(On Diff #10212)	->

In D2394#99805, @elexis wrote:

So the question is what else should be deleted that is also relating to autoflatten (that's where I tuned out last time).
It would be good to get this over with since its one of the few (3?) compiler warnings remaining.

I couldn't find any other reference than there. Only other existing reference was removed in rP11499 It was introduced in rP2022 I believe Ykkrosh(Philipp`) wanted to implement terrain flattening later on but never did #2264 D1642

source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
175 ↗	(On Diff #10212)	Does not work castShadow is not a pointer type. * is overloaded.

In D2394#99809, @Stan wrote:

In D2394#99805, @elexis wrote:

So the question is what else should be deleted that is also relating to autoflatten (that's where I tuned out last time).
It would be good to get this over with since its one of the few (3?) compiler warnings remaining.

I couldn't find any other reference than there. Only other existing reference was removed in rP11499 It was introduced in rP2022 I believe Ykkrosh(Philipp`) wanted to implement terrain flattening later on but never did #2264 D1642

Introduced in rP2022, removed in rP2816, removed more in rP11499, rest removed in D2394 according to a search in source/ for autoflatten.

Does a clean compile on clang.

I see castshadow is still alive and well in source/ and art/.

Found no XML files with autoflatten but many for castshadow.

A bit strange to have castshadow and castshadows, but whatever.

However now I see it's part of:

// Do any necessary conversions into the most recent format:

So the code is actually not to parse legitimate actors, but to convert old ones into the new format.

So therefore it converts castshadows to castshadow.
And there is only one occurrence of castshadows in the entire codebase, so one may even consider removing that too if the use case is solely to convert "old" files.
If one can create new files with castshadows other than making a typo, then (only then?) it would be useful to keep.

But setting actor.add("autoflatten", ""); to true is definitely incorrect if there is no code parsing autoflatten.

Test that you can open a bunch of old actors. you can find them here: https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/official/art/actors?rev=2049&order=name

You performed that test? Got a specific actor of that format?
One puts in the file into the actor folder, starts the actor viewer and then selects that thing and the conversion code is ran?

source/tools/atlas/AtlasUI/ActorEditor/ActorEditor.cpp
175 ↗	(On Diff #10212)	I see

One puts in the file into the actor folder, starts the actor viewer and then selects that thing and the conversion code is ran?

That code is actually only run in the Actor Editor not the Actor Viewer. Atlas is totally unable to load the old actors... Any attempts would end in a few warnings like those:

ERROR: RelaxNGValidator: Validation error: art/actors/structures/fndn_4x42_old.xml:6: Element variant has extra content: texture
ERROR: RelaxNGValidator: Validation error: art/actors/structures/fndn_4x42_old.xml:4: Element group has extra content: variant
ERROR: RelaxNGValidator: Validation error: (null):0: Extra element group in interleav
ERROR: RelaxNGValidator: Validation error: art/actors/structures/fndn_4x42_old.xml:3: Element actor failed to validate content
ERROR: RelaxNGValidator: Validation failed for '(null)'
ERROR: CXeromyces: failed to validate XML file art/actors/structures/fndn_4x42_old.xml
ERROR: CObjectManager::FindObjectBase(): Cannot find object 'structures/fndn_4x42_old.xml'
ERROR: RelaxNGValidator: Validation error: art/actors/structures/fndn_4x4_old.xml:6: Expecting element actor, got Object
ERROR: RelaxNGValidator: Validation failed for '(null)'
ERROR: CXeromyces: failed to validate XML file art/actors/structures/fndn_4x4_old.xml
ERROR: CObjectManager::FindObjectBase(): Cannot find object 'structures/fndn_4x4_old.xml'

Assuming you mean the Actor Editor then yes, you can open any file on the disk (it doesn't even have to be in the 0ad folders though it's better) and it will try converting it.