HomeWildfire Games

Cleanup TileClass prototype and use vector arguments for countInRadius…
AuditedrP21026

Description

Cleanup TileClass prototype and use vector arguments for countInRadius, countMembersInRadius, countNonMembersInRadius, refs #4992.

Details

Auditors
FeXoR
Group Auditors
Restricted Owners Package
Committed
elexisJan 27 2018, 5:37 AM
Parents
rP21025: Stop identifying TileClasses by a customly defined TileClassID, but just…
Branches
Unknown
Tags
Unknown

Event Timeline

smiley raised a concern with this commit.Dec 14 2018, 1:43 PM
smiley added a subscriber: smiley.
smiley added inline comments.
/ps/trunk/binaries/data/mods/public/maps/random/rmgen/tileclass.js
124

Math.min(Math.ceil(position.x + radius), this.size - 1);

This commit now has outstanding concerns.Dec 14 2018, 1:43 PM

Concern raised to keep track of this until freezes lifted.

FeXoR added a subscriber: FeXoR.Dec 14 2018, 2:26 PM
FeXoR added inline comments.
/ps/trunk/binaries/data/mods/public/maps/random/rmgen/tileclass.js
124

Good catch!

(IMO it would be better to create a patch for this than raising a concern. Since this is a plain bug doesn't it break things whenever used?)

smiley added inline comments.Dec 14 2018, 2:30 PM
/ps/trunk/binaries/data/mods/public/maps/random/rmgen/tileclass.js
124

I assumed that this is small enough for a direct commit.
It wont break anything, just make them much slower.

FeXoR added inline comments.Dec 14 2018, 3:11 PM
/ps/trunk/binaries/data/mods/public/maps/random/rmgen/tileclass.js
124

Oh, you're right. I agree in both ;)

elexis marked 4 inline comments as done.Dec 28 2018, 3:01 PM
elexis added a subscriber: nani.
elexis added inline comments.
/ps/trunk/binaries/data/mods/public/maps/random/rmgen/tileclass.js
104

This performance lock / wrong number must vanish asap, possibly the unused slower code with it.

111

Wondering why this equation differs from the one below.
The TileClasses work on tiles, i.e. the grid has the size map_size² (whereas the heightgrid coordinates are vertices, i.e. (mapsize+1)²).
So the first tile index is 0, the last tile index is mapSize - 1.
I'm not sure if it's currently the case, but the TileClasses grid could also be used on (size+1)² for convenience, which would make the line below wrong?

@nani wasn't this maxX vs xMax difference the reason why the 27 removal gave an OOS (when played with unpatched code)?

124

merry xMax then

nani added inline comments.Dec 28 2018, 4:22 PM
/ps/trunk/binaries/data/mods/public/maps/random/rmgen/tileclass.js
111

I think you are correct, partly. Maybe
TileClass.prototype.add and TileClass.prototype.remove are also to blame as they might not always update this.rangeCount[position.y].add accordingly with this.inclusionCount[position.x][position.y].

smiley removed an auditor: smiley.Mar 30 2019, 9:33 PM
This commit no longer requires audit.Mar 30 2019, 9:33 PM
elexis added an auditor: Restricted Owners Package.Mar 30 2019, 9:47 PM
This commit now requires audit.Mar 30 2019, 9:47 PM
wraitii added a subscriber: wraitii.

(to not forget)

(There have been a release *after* this was found)

smiley removed a subscriber: smiley.Jun 25 2019, 5:46 PM

smiley raised a concern with this commit. Dec 14 2018, 13:43

Stan added a subscriber: Stan.Jun 25 2019, 6:10 PM

Let's do better than this: D2010.

FeXoR accepted this commit.Aug 9 2019, 1:42 PM
FeXoR added a subscriber: smiley.

I think it's reasonable to see this as fixed in rP22635 (as @smiley said himself in D2010) so I'm removing the audit.

elexis removed an auditor: wraitii.Aug 9 2019, 1:42 PM
All concerns with this commit have now been addressed.Aug 9 2019, 1:42 PM
FeXoR added a comment.Aug 9 2019, 1:46 PM
This comment was removed by FeXoR.