Page MenuHomeWildfire Games

Atlas variation submenu
Needs ReviewPublic

Authored by trompetin17 on Jul 5 2019, 6:32 PM.
This revision needs review, but all reviewers have resigned.

Details

Reviewers
Silier
Trac Tickets
#2472
Summary

Enable Variation panel, map writer, map reader and CUnit actorSelections


D2752 is not technically a prerequisite, but you can't test on catalina without it.


Test Plan

Apply patch:
create new map, add entity, play with variation, save and reload the map
Load a map, play tiwh variation, save and reload the map

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8213
Build 13385: Vulcan BuildJenkins
Build 13384: arc lint + arc unit

Event Timeline

trompetin17 created this revision.Jul 5 2019, 6:32 PM
trompetin17 edited the test plan for this revision. (Show Details)Jul 5 2019, 6:33 PM
Vulcan added a comment.Jul 5 2019, 6:34 PM

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

Linter detected issues:
Executing section Source...

source/graphics/MapReader.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

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

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 211| »   {
|    | [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/1901/display/redirect

trompetin17 updated this revision to Diff 8726.Jul 5 2019, 6:40 PM

Fixing std::string initialize and check empty

Silier added a subscriber: Silier.Jul 5 2019, 6:40 PM

please remove not needed braces for one command after if condition, will test once it builds

source/graphics/MapWriter.cpp
394–395

please remove { }

420

remove { }

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
336

remove { }

375

remove { }

Vulcan added a comment.Jul 5 2019, 6:41 PM

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

Linter detected issues:
Executing section Source...

source/graphics/MapReader.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

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

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 211| »   {
|    | [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/1902/display/redirect

trompetin17 updated this revision to Diff 8727.Jul 5 2019, 6:44 PM

remove not needed braces for one command after if condition,

trompetin17 marked 3 inline comments as done.Jul 5 2019, 6:45 PM
Vulcan added a comment.Jul 5 2019, 6:45 PM

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

Linter detected issues:
Executing section Source...

source/graphics/MapReader.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

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

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 211| »   {
|    | [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/1903/display/redirect

trompetin17 updated this revision to Diff 8732.Jul 5 2019, 6:51 PM

Update Copyright

Silier added inline comments.Jul 5 2019, 6:54 PM
source/graphics/MapReader.cpp
1018

I believe same here about { }

1072

this { } can go away

Vulcan added a comment.Jul 5 2019, 6:55 PM

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

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 211| »   {
|    | [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/1905/display/redirect

Silier added a comment.Jul 5 2019, 6:57 PM

It builds for me on windows but not with jenkins, he has linux

source/graphics/MapReader.cpp
31

jenkins does not like this include

../../../source/graphics/MapReader.cpp:31:27: fatal error: graphics/unit.h: No such file or directory

 #include "graphics/unit.h"
trompetin17 updated this revision to Diff 8733.Jul 5 2019, 7:24 PM

Fixing unit.h to Unit.h, remove some braces

trompetin17 marked 4 inline comments as done.Jul 5 2019, 7:25 PM
Vulcan added a comment.Jul 5 2019, 7:26 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
| 211| »   {
|    | [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/1907/display/redirect


Evidence about variation!

trompetin17 updated this revision to Diff 8745.Jul 6 2019, 8:58 AM

Change atribute variations to node element

trompetin17 updated this revision to Diff 8746.Jul 6 2019, 9:01 AM

Remove backspace

Vulcan added a comment.Jul 6 2019, 9:32 AM

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

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 211| »   {
|    | [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/1917/display/redirect

Vulcan added a comment.Jul 6 2019, 9:35 AM

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

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 211| »   {
|    | [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/1918/display/redirect

elexis added a subscriber: elexis.Jul 8 2019, 5:50 PM

Thanks for having taken into account my IRC review remarks in the past days about the "|" concatenation in the attribute setting vs. individual XML nodes.

I consulted the findings from rP22335 again to see if UTF8 handling is proper, seems so.

When opening the Arcadia map:

XeroXMB.cpp(257): Assertion failed: "id < m_Size && "Element ID out of range""
Assertion failed: "id < m_Size && "Element ID out of range""
Location: XeroXMB.cpp:257 (operator[])
elexis added inline comments.Jul 14 2019, 9:59 PM
source/graphics/MapReader.cpp
1018

I suspect it complains about childActors[0] being out of bounds

trompetin17 updated this revision to Diff 8906.Jul 15 2019, 5:42 AM

Add validation on childActors, it must have one child

trompetin17 marked an inline comment as done.Jul 15 2019, 5:43 AM

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

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|1055| »   »   »   oldObjects.push_back(obj);
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: obj.owner

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 211| »   {
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

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

elexis added inline comments.Jul 15 2019, 12:53 PM
source/graphics/MapReader.cpp
1014

it warns about > 1, perhaps it should also warn about 0?

Perhaps it would be good to display the names of the variations. For example if one selects a cavalry unit, there are a range of colors settings, but one can't deduce which color refers to which property of the model.
(It's a bit unexpected that one can chose whether it's carrying meat or running, but maybe that's technically impossible to check)
Perhaps also more horizontal space for the panel, it looks a bit squeezed.

Very nice feature!

trompetin17 added inline comments.Jul 15 2019, 3:35 PM
source/graphics/MapReader.cpp
1014

could be, but if we have 0 children that means they didn't choose any variant and this could be acceptable

trompetin17 edited the summary of this revision. (Show Details)Jul 19 2020, 1:42 AM

Update patch with the current version.
When an empty selection hide combobox in variation panel
Change space in object panel to use 50% space

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

Linter detected issues:
Executing section Source...

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/VariationControl.h
|   1| /*·Copyright·(C)·2009·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2009"

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/VariationControl.h
|  25| class·VariationControl·:·public·wxScrolledWindow
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classVariationControl:' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/AtlasUI/ScenarioEditor/Tools/Common/ObjectSettings.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2017"

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/VariationControl.cpp
|   1| /*·Copyright·(C)·2011·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2011"

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/Object.cpp
| 508| BEGIN_EVENT_TABLE(PlayerComboBox,·wxComboBox)
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If BEGIN_EVENT_TABLE is a macro then please configure it.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|1056| »   »   »   oldObjects.push_back(obj);
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: obj.owner

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
| 211| »   if·(cmpVisualActor)
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

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

Silier requested changes to this revision.EditedJul 19 2020, 6:02 PM

First, this is great feature to have. But it needs a bit of work.

1.) It should be possible to set none variant in a group as in some cases they compete with variants of other groups and one will never see the variants from another groups.
2.) Variant menu needs to should be visible in actor viewer too as mostly that is the place where people try animations and stuff.
Edit:
After longer consideration 1 is not required as actor will pick default one in group anyway.
But I still think when we are enabling variation menu it should be enabled in whole atlas, not just one part of it.

This revision now requires changes to proceed.Jul 19 2020, 6:02 PM

Also please update years

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/VariationControl.cpp
1 ↗(On Diff #12778)

2020

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/VariationControl.h
1 ↗(On Diff #12778)

2020

source/tools/atlas/AtlasUI/ScenarioEditor/Tools/Common/ObjectSettings.cpp
1 ↗(On Diff #12778)

2020

In D2042#124775, @Angen wrote:

First, this is great feature to have. But it needs a bit of work.

1.) It should be possible to set none variant in a group as in some cases they compete with variants of other groups and one will never see the variants from another groups.
2.) Variant menu needs to be visible in actor viewer too as mostly that is the place where people try animations and stuff.

Angen first at all, thx for taking time to review, I'm wondering about the point 1, because currently, we don't support getting object settings from the group, just take the first element in the selection. So I think we need to think about how to handle correctly multiple selections because you can have different scenarios, for example, the same template multiple units, but also, multiple templates with multiple units.

Any suggestion to drive the correct implementation


I meant this.
Build variant will not have effect because it competes with skirmisher-shield-fast.
So my requirement was to be able to set here nothing.

Silier resigned from this revision.Jul 31 2023, 5:26 PM
This revision now requires review to proceed.Jul 31 2023, 5:26 PM