Paths

Table of Contentst

Change clearance of large land units from 4 to 3, large ships from 12 to 10
ClosedPublic
Actions

Authored by wraitii on Mar 23 2017, 5:56 PM.

Details

Reviewers

elexis
fatherbushido

Commits

rP19365: Change clearance of large land units from 4 to 3, clearance of large ships from…

Summary

This is a change I've done in my unit motion rewrite branch to improve the fluidity of large unit movements. It changes the clearance of large land units (elephants, siege mostly) from 4 to 3 (regular units are 1).

This makes them considerably easier to use imo.
Some comparison screenshots: as you can see, it fits the elephants much better. On most siege, it fits their width, not their length. This is imo a good change because they can now pass through areas where it looks like they should pass, since they should in theory be able to pass through any hole wide than their width, not their length. However, for the helepolis, this will lead to clipping.
We could in principle add a new passability class for the very-large-units, but I don't think it's worth the perf hit tbh.

Before:

After:

Comparison from the front: it fits the width, not the length.

Before:

After:

before:

after:

Test Plan

Check out the screenshots, check out the in-game behaviour, report.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

wraitii created this revision.Mar 23 2017, 5:56 PM

Owners added a subscriber: Restricted Owners Package.Mar 23 2017, 5:56 PM

wraitii edited the summary of this revision. (Show Details)Mar 23 2017, 5:57 PM

wraitii edited the summary of this revision. (Show Details)

wraitii edited the summary of this revision. (Show Details)Mar 23 2017, 6:00 PM

elexis added a reviewer: elexis.Mar 23 2017, 6:25 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/580/ for more details.

Harbormaster completed remote builds in B855: Diff 907.Mar 23 2017, 7:05 PM

For most siege engines the change seems fine, some notable exceptions:

elephants, that seems pretty meh

macedonian siege ram is very wide, more than most other rams. ugly but remotely acceptable. notice that the average case is the worst case, since one often has multiple of them selected and moves them to some target, so they end up all in one place overlapping:

roman rams don't seem relevantly worse

The rear end of catapults had been out of bounds before as well, so not a regression:

siege towers were bad, now worse. they are not that often trained in comparison to rams

Reducing the size of models could be done, but that would further reduce the epicness of siege engines who were historically much bigger than portrayed in our game.

How big is the memory impact actually when adding another class? Should consider 1500 units on a normal sized mainland map with many buildings and then print the size of the affected grids.

Re elephants: notice that the butt is fully inside the square. If we moved the elephant back a bit, the clipping would be less obvious in front.
Anyhow, imo this type of clipping is a game feature, and unless you want to go and implement complete collision detections for all models I don't see an easy fix :P. Furthermore, units should be able to pass where it looks like they should be able to pass, and in that respect (particularly for Elephant) this fits much better.

Agreed that siege towers are the biggest issue here.

Re costs. I could be wrong here but I don't believe there is actually a memory cost since we'd still be under 16 bits (one per passability class). However any new passability class means that when we need to rasterise something, we have to do it once more. All passability maps are also over-rasterised up by "max clearance" to ensure no problems, but this won't come into play here since ships have a class with clearance 12.
Overall, it wouldn't be a huge perf hit to add a new class for very-large units, but it would be slower. Since imo only siege towers need it I'm not entirely sure it's worth it - but we can give it a shot.

The best way to know if that is acceptable is imo to hardly test that in games.
My opinion is not really valuable for the graphical side though.

This revision is now accepted and ready to land.Mar 27 2017, 2:24 PM

As mentioned by fatherbushido, that issue wouldn't be a surprise to players. We should aim for fixing those as far as possible though:

We should find an actual estimation on how much performance loss adding a new class would have sometime. Maybe not now. Convinced it will be a measurable performance impact and we definitely don't have a use for adding further bottlenecks to the current state of laggiess.

The siege tower issue is the worst case of this patch and is beyond acceptable but tolerable to me.

While at it, we should also consider ships as fatherbushido mentioned in IRC.
In about every naval skirmish, people rage about ships being stupid, not abiding orders. It's exactly the same issue as with land units clearance.
After some testing, it would be great to equally reduce the clearance of big ships by 25%, i.e. from 12 to 8, or even only to 10.

For the trireme it is quite obvious:
Before:

After:

For the medium war ship, we already have observable collisions that would become worse. So perhaps 10 is better from a visual pov. If we can tolerate the siege tower, maybe we can tolerate 8 here too, not sure:
Before:

After:

The small-ship class with 5 shouldn't be reduced probably:

10 for ships is the truth (TM):

I'm actually more worried about the balancing impact if we were to make it 8.
Especially notice that the nr of ships per area increases by the square of the changed clearance, not proportional.
Also looks like we implemented half of the ship ramming feature :D
pathfinder.xml in the _test.sim mod looks like it doesn't need an update.

This revision now requires changes to proceed.Mar 31 2017, 6:08 AM

Updated, RC.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/661/ for more details.

Harbormaster completed remote builds in B970: Diff 1034.Mar 31 2017, 4:32 PM

Thanks. Will playtest soon :-)

This revision is now accepted and ready to land.Mar 31 2017, 4:43 PM

Closed by commit rP19365: Change clearance of large land units from 4 to 3, clearance of large ships from… (authored by wraitii). · Explain WhyMar 31 2017, 4:50 PM

This revision was automatically updated to reflect the committed changes.

elexis mentioned this in rP21348: Decrease a bit the clearance of fishing ships (from 4 to 3)..Feb 24 2018, 3:06 PM