Page MenuHomeWildfire Games

Jebel Barkal wall fix
ClosedPublic

Authored by temple on Mar 29 2018, 2:45 AM.

Details

Summary

Just a small math error: distributePointsOnCircularSegment uses the formula startAngle + maxAngle * i / pointCount for i < pointCount, so the last angle is startAngle + maxAngle * (pointCount - 1) / pointCount. We want this to be gridStartAngle + gridMaxAngle for pointCount = gridPointsX, which means we need to use maxAngle = gridMaxAngle * gridPointX / (gridPointsX - 1).
Also the placeCircularWall arguments were in the wrong order.

Test Plan

Before:

After:

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

temple created this revision.Mar 29 2018, 2:45 AM
temple added inline comments.
binaries/data/mods/public/maps/random/jebel_barkal.js
686 ↗(On Diff #6286)

Currently gridPointsX is defined as layoutKushTemples.length, so this is just i.

lyv added a subscriber: lyv.Mar 29 2018, 8:54 AM
lyv added inline comments.
binaries/data/mods/public/maps/random/jebel_barkal.js
851 ↗(On Diff #6286)

Perhaps everything below true can be ommited, they are the default values.

elexis accepted this revision.Mar 29 2018, 12:06 PM

As mentioned in the starting position patch, the distribute circle segment function can indeed return the first position at startAngle and the last at maxAngle.
Then we can avoid this +1 / -1 thing here.
The healer circle on this map would also benefit.

I didn't test.
Thanks for fixing this temple, it was one of the greatest visual flaws in this map.

Offtopic 1:
I notice on your screenshots that the water is rendered on the minimap but not ingame.
It sounds like Vladislav watershader fix might help with that.

Offtopic 2:
We can also observe that the wallGridMaxAngleSummand is not ideal, as the walls should encompass the temples.
The most correct number would be computed using trigonometry, but a better magic number would also be an improvement.

binaries/data/mods/public/maps/random/jebel_barkal.js
639 ↗(On Diff #6286)

Ah, that's why I couldn't get the wall right.

686 ↗(On Diff #6286)

The https://en.wikipedia.org/wiki/Single_source_of_truth pattern is important. So either we assume that these two variables are identical in nature, then we should delete one of them, or we assume the possibility that they differ, in which case we keep the code as is.

840 ↗(On Diff #6286)

(This equation was meaningless written by a helpless dev)

851 ↗(On Diff #6286)

IMO default values are an anti-pattern. Requiring authors to pass a value in every call means they have to consider each value in each call, whereas defaults often lead to forgetting to consider these values. Also it adds a bit of indirection when one wants to lookup the value used.

This revision is now accepted and ready to land.Mar 29 2018, 12:06 PM
In D1421#58208, @elexis wrote:

Offtopic 1:
I notice on your screenshots that the water is rendered on the minimap but not ingame.
It sounds like Vladislav watershader fix might help with that.

I was zoomed out quite a bit, and overhead.

This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Mar 30 2018, 8:21 PM