Page MenuHomeWildfire Games

Store Garrison(Holder) in map file.
ClosedPublic

Authored by Freagarach on Jan 23 2020, 9:52 PM.
Tags
None
Subscribers
Restricted Owners Package
Restricted Owners Package
Restricted Owners Package
Tokens
"Like" token, awarded by elexis.

Details

Summary

Currently the garrison of an entity ought to be stored seperately in the mapfile as a script. That is doable.
However, D1958 allows entities to be created on init, thus when an entity is defined in a map it will create another one on map load (using the entity ID +1, creating havoc).
The only proper way of solving this would probably be to define that there is already an entity on the position.

This patch aims at fixing that the entities garrisoned in Atlas are stored and (re)garrisoned at init.


See also:


Possible extensions:

  • Make a UI to set garrisoned entities in Atlas.
  • Store more properties (such as turret position) in the mapfile.
Test Plan
  • Verify that a map with Garrison in a structure correctly saves that in the map-file.
  • Verify that loading a map with Garrison in an entity (such as Silica Nomad) correctly loads.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Freagarach planned changes to this revision.EditedFeb 2 2020, 1:24 PM

Is it necessary to store the garrisonHolder also? It adds more room for errors and seems to add nothing really.
One could also add the garrisonHolder when adding the entities on game init.

source/graphics/MapReader.cpp
1

++

957

entity_id_t

Freagarach updated this revision to Diff 11264.Feb 3 2020, 8:14 PM
  • Bump year in MapReader.
  • Remove GarrisonHolder from map.
Stan added a subscriber: Stan.Feb 3 2020, 8:34 PM
Stan added inline comments.
binaries/data/mods/public/maps/skirmishes/Sicilia_Nomad.xml
15599 ↗(On Diff #11264)

I'm wondering if it shouldn't also be uid somehow? to make the link between entities.

source/graphics/MapReader.cpp
1011

I guess you could set the size of Garison to avoid resizing since you already know the definite size.

Can you use a range for loop?

Something along the lines of

for (const XMBElement& garr_ent : garrison) {
  XMBAttributeList attrs = garr_ent.GetAttributes();
  Garrison.insert(attrs.GetNamedItem(at_gid).ToInt());
}
source/graphics/MapWriter.cpp
351

Range loop here too.

source/simulation2/components/ICmpGarrisonHolder.cpp
38

range based for loop?

45

Range based for loop?

Vulcan added a comment.Feb 3 2020, 8:47 PM

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

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpGarrisonHolder.h
|  23| class·ICmpGarrisonHolder·:·public·IComponent
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classICmpGarrisonHolder:' 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/1705/display/redirect

Silier requested changes to this revision.Feb 4 2020, 11:55 AM
Silier added a subscriber: Silier.
Relax-NG validity error : Extra element Entities in interleave
/public/maps/skirmishes/Sicilia_Nomad.xml:0: Relax-NG validity error : Element Scenario failed to validate content
This revision now requires changes to proceed.Feb 4 2020, 11:55 AM

(To be fixed in ./binaries/data/mods/public/maps/scenario.rng ./binaries/data/mods/public/maps/scenario.rnc )

Freagarach marked 4 inline comments as done.Feb 4 2020, 8:36 PM
Freagarach added inline comments.
source/graphics/MapReader.cpp
1011

I guess you could set the size of Garison to avoid resizing since you already know the definite size.

Hmm, I could not find a way to set the size actually?
(https://stackoverflow.com/questions/21930242/are-there-functions-to-request-capability-for-stdset)

Freagarach updated this revision to Diff 11270.Feb 4 2020, 8:36 PM
  • Fix checkref warnings.
  • Use range for-loops.
Vulcan added a comment.Feb 4 2020, 9:00 PM

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

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpGarrisonHolder.h
|  23| class·ICmpGarrisonHolder·:·public·IComponent
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classICmpGarrisonHolder:' 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/1710/display/redirect

by this, code in /helpers/Setup.js using Garrison should be removed, as there should be only one way how to define the same data in one file.

source/simulation2/components/ICmpGarrisonHolder.cpp
34

does this need to require set ? if code could use vector, that copying could be avoided

46

you can remove these brackets :)

47

btw, this could pass array of them, no ? Instead one by one

Freagarach marked 2 inline comments as done.
  • Removed different manner of storing garrisoned entities.
  • Throw error when trying to garrison an entity in a entity that is not capable of holding entities.
  • Use std::vector in getEntities.
source/simulation2/components/ICmpGarrisonHolder.cpp
47

I tried that at first but it didn't work.

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

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpGarrisonHolder.h
|  23| class·ICmpGarrisonHolder·:·public·IComponent
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classICmpGarrisonHolder:' 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/1759/display/redirect

btw, this could pass array of them, no ? Instead one by one

I tried that at first but it didn't work.

It works

virtual void SetInitEntities(std::vector<entity_id_t> entities)
{
   m_Script.CallVoid("AddInitGarrisonEntity", entities);
}

You just need to change type of Garrison from set to vector and edit AddInitGarrisonEntity to work with array.
And one bonus. When Garrison is vector, you can use resize to do what Stan wanted.

Freagarach planned changes to this revision.Feb 20 2020, 7:54 PM
Freagarach updated this revision to Diff 11411.Feb 21 2020, 7:37 PM
Freagarach marked 2 inline comments as done.

Use std::vector.

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

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpGarrisonHolder.h
|  23| class·ICmpGarrisonHolder·:·public·IComponent
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classICmpGarrisonHolder:' 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/1803/display/redirect

Silier requested changes to this revision.Feb 21 2020, 9:41 PM
Silier added inline comments.
binaries/data/mods/public/simulation/components/GarrisonHolder.js
698

The name says add but comment and code says set

710

this is changing postcondition of the function.
this.initGarrison was always undefined after this function was called, now it is not the case.

source/graphics/MapReader.cpp
1059

can be just if (Garrison.size())

This revision now requires changes to proceed.Feb 21 2020, 9:41 PM
Stan added inline comments.Feb 22 2020, 10:45 AM
source/graphics/MapReader.cpp
1065

Maybe it should state that it has no garrisonholder component. One could also display the player set above if available.

Freagarach updated this revision to Diff 11440.Feb 27 2020, 6:08 PM
Freagarach marked 4 inline comments as done.
Freagarach edited the test plan for this revision. (Show Details)
  • SetInitGarrison.
  • if (Garrison.size()).

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

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpGarrisonHolder.h
|  23| class·ICmpGarrisonHolder·:·public·IComponent
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classICmpGarrisonHolder:' 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/1823/display/redirect

It would be nice to solve the whole ticket here. Are you up to that task?

Silier added inline comments.Mar 1 2020, 1:45 PM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
700

should clone to avoid issue from D2562 when some script decides to spawn entity with garrisoned entities and uses this function for it.

Silier requested changes to this revision.Mar 1 2020, 1:46 PM
This revision now requires changes to proceed.Mar 1 2020, 1:46 PM
Freagarach updated this revision to Diff 11452.Mar 1 2020, 3:34 PM
Freagarach marked an inline comment as done.
  • Clone.

To answer your question @Angen, I do not consider myself up to that task (Atlas is written in C++) and I personally think smaller incremental patches are both easier to review and better for morale ;)

Vulcan added a comment.Mar 1 2020, 4:00 PM

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

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpGarrisonHolder.h
|  23| class·ICmpGarrisonHolder·:·public·IComponent
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classICmpGarrisonHolder:' 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/1833/display/redirect

Silier accepted this revision as: Silier.Mar 7 2020, 11:31 AM

Minimal code.
Starting entities are garrisoned correctly.
Saving map in atlas again does not rewrite Garrison element.
There is no other map to be updated.
Single way how to define garrisoned entities in map file.

This revision is now accepted and ready to land.Mar 7 2020, 11:31 AM
Silier removed a reviewer: Restricted Owners Package.Mar 7 2020, 11:32 AM

(can be fixed when committing and/or ignored)

source/graphics/MapReader.cpp
1010

(resize -> reserve, makes it a bit faster as it doesnt init each element)

1059

(.empty)

1065

(Map error in MapReader -> CXMLReader::ReadEntities, compare with other LOGERRORs)

source/graphics/MapWriter.cpp
351

(entity_id_t& -> entity_id_t, supposedly copying an integer is faster than passing a reference to an integer, I guess because the former avoids a dereference operation and it means the value is copied into the current memory page)

source/simulation2/components/ICmpGarrisonHolder.cpp
37

>> -> > >

source/simulation2/components/ICmpGarrisonHolder.h
22

\n
#include <vector>

29

const ref?

elexis added inline comments.Mar 7 2020, 1:31 PM
source/graphics/MapReader.cpp
1010

Is this code wrong?
resize creates N elements, and then N elements are appended using pushed_back, doesn't that make 2N elements with the first N being zero?

Freagarach marked 8 inline comments as done.Mar 8 2020, 8:45 AM
Freagarach added inline comments.
source/graphics/MapReader.cpp
1010

True.

1065

(Not in this file.)

Freagarach updated this revision to Diff 11463.Mar 8 2020, 8:45 AM
Freagarach marked 2 inline comments as done.
  • Reserve size for Garrison.
  • Check for emptyness explicitly.
  • Other issues mentioned by @elexis.
Vulcan added a comment.Mar 8 2020, 9:19 AM

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

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpGarrisonHolder.h
|  25| class·ICmpGarrisonHolder·:·public·IComponent
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classICmpGarrisonHolder:' 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/1839/display/redirect

Freagarach closed this revision.Mar 15 2020, 5:08 PM

Thanks for the review and commit @Angen :)