Page MenuHomeWildfire Games

deprecate Mechanical class
ClosedPublic

Authored by Nescio on Aug 20 2019, 10:51 AM.

Details

Summary

This patch deprecates the unnecessary Mechanical class.
The Mechanical visible class is not particularly useful: it is basically the union of Siege and Ship classes. Mechanical is only used in four auras, one of which is corrected to Siege, as it should be, the other three replace it with "Siege", "Ship".
Furthermore, the mechanical_ part was removed from template file names in D1760/rP22204.

Test Plan

Check for mistakes.

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.Aug 20 2019, 10:51 AM

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

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

wraitii accepted this revision.Aug 24 2019, 11:06 AM

Seems good, Mechanical was only useful as "not organic", but we also can write '!Organic'.

binaries/data/mods/public/simulation/components/Identity.js
68 ↗(On Diff #9414)

Is having such a list really useful?

binaries/data/mods/public/simulation/data/auras/units/heroes/gaul_hero_brennus.json
4 ↗(On Diff #9414)

From the description, shouldn't this also apply to structures?

This revision is now accepted and ready to land.Aug 24 2019, 11:06 AM
Nescio added inline comments.
binaries/data/mods/public/simulation/components/Identity.js
68 ↗(On Diff #9414)

Don't ask me. @fatherbushido reminded me somewhere in the past that this list should be updated, though.

binaries/data/mods/public/simulation/data/auras/units/heroes/gaul_hero_brennus.json
4 ↗(On Diff #9414)

Yeah, I wondered about that too. But wouldn't that be a gameplay change?

Nescio added inline comments.Aug 24 2019, 11:26 AM
binaries/data/mods/public/simulation/data/auras/units/catafalques/mace_catafalque.json
3 ↗(On Diff #9414)

["Unit", "Structure"] is actually shorter, but I suppose animals should not be included.

wraitii added inline comments.Aug 31 2019, 1:39 PM
binaries/data/mods/public/simulation/data/auras/units/heroes/gaul_hero_brennus.json
4 ↗(On Diff #9414)

Yeah I suppose it would. I guess it might be fairer to change the description.

Nescio added inline comments.Aug 31 2019, 1:42 PM
binaries/data/mods/public/simulation/data/auras/units/heroes/gaul_hero_brennus.json
4 ↗(On Diff #9414)
This revision was automatically updated to reflect the committed changes.
elexis added a subscriber: elexis.Aug 31 2019, 2:46 PM
elexis added inline comments.
binaries/data/mods/public/simulation/components/Identity.js
68 ↗(On Diff #9414)

I don't see that this is wrapped in <example>, so it's not arbitrary.
The text says Choices include: X with a removed class being an element of set X, so I suppose it would be right to keep the text right.
Whether the set should be complete is a different question, I suppose it doesn't hurt if it is and if having an entry there helps, then that argument would apply to every occurring class alike.

wraitii added inline comments.Aug 31 2019, 2:50 PM
binaries/data/mods/public/simulation/components/Identity.js
68 ↗(On Diff #9414)

This sounds vaguely like it should be updated to be a Choice then, but that definitely breaks all mods.