Page MenuHomeWildfire Games

CivSpecific class for buildable civ-specific structures
ClosedPublic

Authored by Nescio on May 13 2020, 10:05 PM.

Details

Summary

This patch introduces a CivSpecific class for those specific structures that should be included in the {civ}.json files and listed in the civilization overview (“History”) page.

Previously there already exist a SpecialBuilding class, however, that one is tied to the template_structure_special,xml file, thus not usable, since not all structures that inherit from *_special are buildable, and since not all civ specific structures inherit from *_special. Therefore an additional class is necessary.

For the Roman siege walls, the class is inserted in the wallset file, which also shows up in the Structure Tree, not in the individual wall segment templates.

According to simulation/components/Identity.js, <Classes> is supposed to be listed below <Tooltip> but above <Icon> and <RequiredTechnology>.

[EDIT] Furthermore, changed the caption strings in the civ overviews from “Special Structure/s” and “Special Technology/ies” into “Specific Structure/s” and “Specific Technology/ies”, for consistency.

Test Plan

Check for mistakes and omissions.

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

Nescio created this revision.May 13 2020, 10:05 PM
Owners added a subscriber: Restricted Owners Package.May 13 2020, 10:05 PM

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

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

Nescio planned changes to this revision.Jul 14 2020, 1:19 PM
Nescio updated this revision to Diff 12933.Jul 27 2020, 10:41 AM
  • rebased and updated

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

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

bb added a subscriber: bb.Sep 25 2020, 5:41 PM

One obstruction for adding this considering the maintainability.

What would be the use case of this class?

What would be the use case of this class?

https://code.wildfiregames.com/D2720#115075

bb added a comment.Sep 25 2020, 11:09 PM

Ahh okay, thanks, needs @s0600204 then...

Nescio updated this revision to Diff 14983.Jan 4 2021, 1:45 PM
  • rebased and updated
Vulcan added a comment.Jan 4 2021, 1:47 PM

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

builderr-debug-macos.txt
fatal error: file '/Users/wfg/Jenkins/workspace/macos-differential/build/workspaces/gcc/../../../libraries/source/spidermonkey/include-unix-debug/mozilla/StaticAnalysisFunctions.h' has been modified since the precompiled header 'obj/network_Debug/precompiled.h.gch' was built
note: please rebuild precompiled header 'obj/network_Debug/precompiled.h.gch'
1 error generated.
make[1]: *** [obj/network_Debug/NetMessageSim.o] Error 1
make: *** [network] Error 2

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2675/display/redirect

I guess you want to use this for the civ info screen? Then you need SpecialBuilding rather than CivStructure?

I'm not a fan of CivStructure for the following two reasons:

  1. It sounds too generic - all structures that a civ can build could be described as a "civ structure".
  2. In the civinfo page the structures appear under the subtitle of "Special Buildings". Thus having the class to get them appear there being SpecialBuilding makes logical sense - the class matches the subtitle (and vice-versa). CivStructure seems counter-intuitive.

However I do see your point about the name SpecialBuilding being tied up with template_structure_special (even if it doesn't bestow the class to its descendants anymore).

And there's also no reason why the caption in the civinfo page can't be changed.

I was considering suggesting UniqueStructure as an alternative class name - but that name would seems to imply that the structure may only be built once, which not only wouldn't true, and as some are buildable by more than one civ (eg. "Rotary Mill" which is shared between brit, gaul, and Aristeia's egyptian and judah civs (although the latter two call it "Threshing Floor")) it's not even that they're necessarily unique to any given civ.

I'd like to propose CivSpecificStructure as the class name, along with changing the subtitle and identifying-class in the civinfo page to match.

I was considering suggesting UniqueStructure as an alternative class name - but that name would seems to imply that the structure may only be built once

I don't really agree, RTS have a long history of "unique units", at least Aoe2 and RoN had those.

That being said, think further that in the future, we might have more differentiated civilisations where it doesn't really make sense to talk of "unique structures" or "unique units". I'm not entirely sure this is better than 'hardcoding' it somehow in the civ info.

Yes, I fully agree with matching the caption and the class name; consistency is important.
As for the CivStructure class name, I only proposed it because of lack of anything better. I briefly considered the name CivOverview, to make it clear what the class is for, but decided against because heroes are included in the civ overviews too. I don't mind changing it to CivSpecificStructure, though it's a bit verbose. Other suggestions are welcome, of course.
It's better to avoid the words unique and special, especially because some structures are available to more than one civ (e.g. military colony, library).

some structures are available to more than one civ (e.g. military colony, library).

I would note that this doesn't necessarily mean we shouldn't show them as "unique units" if they are sufficiently special that they're a highlight of a given civ's gameplay.

Nescio added a comment.Jan 5 2021, 1:28 PM

No, I'm not saying they should be removed from the civilization overviews; I was merely pointing out that we ought to avoid calling them “unique” or “special”.

s0600204 requested changes to this revision.Jan 8 2021, 10:47 AM

Well whatever you decide to use @Nescio, could I ask for the revision to be updated?

This revision now requires changes to proceed.Jan 8 2021, 10:47 AM

Sure! I'm just waiting for clear opinions on what the class should be called.

I think CivSpecificStructure is the best we've come up with thus far, despite its length, so unless anyone has any objections (and can suggest a better class name) lets go with that.

Nescio updated this revision to Diff 15069.Jan 9 2021, 12:11 PM
Nescio retitled this revision from CivStructure class for civ-specific structures to CivSpecific class for buildable civ-specific structures.
Nescio edited the summary of this revision. (Show Details)
  • change class name to CivSpecific
  • fix identifying classes in the StructuresSubsection.js so these structures are actually displayed (and not Wonders)
  • replace “special” in subsection headers with “specific” for consistency
Owners added a subscriber: Restricted Owners Package.Jan 9 2021, 12:11 PM

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui_dbg.a(precompiled.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file f

See https://jenkins.wildfiregames.com/job/macos-differential/2727/display/redirect for more details.

s0600204 accepted this revision.Jan 11 2021, 12:55 AM

Thanks.

I would have liked to have retained the Wonder class in the civinfo page, but I won't change that when committing. We'll see what the community says (if they even notice) once A24 is out.

This revision is now accepted and ready to land.Jan 11 2021, 12:55 AM
This revision was automatically updated to reflect the committed changes.

I would have liked to have retained the Wonder class in the civinfo page, but I won't change that when committing. We'll see what the community says (if they even notice) once A24 is out.

Wonders were not included in the civilization overviews in A23 or any previous alpha I remember, so that's why I left them out. Besides, all civs have them, and they're all statistically identical. If people really want to show wonders there, then one could simply insert a CivSpecific class into the temlate_structure_wonder.xml; there is no need for hard-coding the Wonder class there (you don't know what mods want to do).