This patch removes frequency="0" variants from Briton structure actor files, because those lines are unused, unnecessary, and potentially confusing. These tend to be deprecated versions that differed in only one line. Moreover, everything is preserved in the revision history logs.
Also retitled the variant names for consistency.
Details
Details
- Reviewers
Stan - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP23558: Remove deprecated variants from Brit structures actors.
Agree this is an improvement.
Diff Detail
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
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1765/display/redirect
Comment Actions
I don't know if this is a good idea. Some of the removed models in this patch are good. Maybe they even deserve to get their own actors. Applying this patch will result in a lot more of unused art files.
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1767/display/redirect
Comment Actions
I don't know if this is a good idea. Some of the removed models in this patch are good. Maybe they even deserve to get their own actors. Applying this patch will result in a lot more of unused art files.
How come?
If you look at e.g. the Briton structures, typically the only difference is the "0" variant uses
<texture file="structural/celt_struct.dds" name="baseTex"/>
and the "1" variant uses
<texture file="structural/brit_struct.png" name="baseTex"/>
Furthermore, if "0" variants are kept in these files, then why not in the hundreds of other structures?
Comment Actions
They reference dae files, those files will start to show up in checkrefs.pl.
<texture file="structural/celt_struct.dds" name="baseTex"/>
I agree with the texture changes. Not with all the patch :) There are multiple changes here
- Better Variant Names : Good,
- Removing variants with 0 frequency: Generally good with some exceptions.
- Removing references to good models : Bad,
- Removing placeholder textures : Good,
- Probably some other stuff I haven't read the whole patch yet.
Comment Actions
Walls, towers, while not completely historically accurate, the models aren't bad per se.
| binaries/data/mods/public/art/actors/structures/ptolemies/wall_short.xml | ||
|---|---|---|
| 6 ↗ | (On Diff #11360) | This one. |
| binaries/data/mods/public/art/actors/structures/ptolemies/wall_tower.xml | ||
| 5 ↗ | (On Diff #11360) | This one. |
| binaries/data/mods/public/art/actors/structures/ptolemies/wooden_tower.xml | ||
| 6 ↗ | (On Diff #11360) | This one. |
Comment Actions
"0" variants on the left, "1" on the right.
If the former are considered worth preserving, wouldn't it be better to give them files of their own (cf. carthaginians/market_old.xml, ptolemies/house_old.xml), so they'll at least show up in the actor list, rather than hiding them in unused lines?
For comparison:
"0" variants on the left, "1" on the right.
If the former are considered worth preserving, wouldn't it be better to give them files of their own (cf. carthaginians/market_old.xml, ptolemies/house_old.xml), so they'll at least show up in the actor list, rather than hiding them in unused lines?
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1917/display/redirect
| binaries/data/mods/public/art/actors/structures/britons/stable.xml | ||
|---|---|---|
| 5 ↗ | (On Diff #11561) | Forgot to capitalize the S. |
Comment Actions
I'll fix the S when committing, thanks for the patch.
Changes are good, new texture should be used.the rest is mostly duplication that should be nuked