HomeWildfire Games

Cleanup TileClass prototype and use vector arguments for countInRadius…


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


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

Event Timeline

smiley raised a concern with this commit.Dec 14 2018, 1:43 PM
smiley added a subscriber: smiley.
smiley added inline comments.

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.

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

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

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.

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


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)?


merry xMax then

nani added inline comments.Dec 28 2018, 4:22 PM

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.