Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Codechange: Remove assert when trying to intersect two tile areas and… #7238

Closed
wants to merge 1 commit into from

Conversation

@J0anJosep
Copy link
Contributor

@J0anJosep J0anJosep commented Feb 16, 2019

… one of them is empty.

@PeterN
Copy link
Member

@PeterN PeterN commented Feb 16, 2019

When would this assert trigger currently?

@J0anJosep
Copy link
Contributor Author

@J0anJosep J0anJosep commented Feb 17, 2019

On master, it doesn't. I think it doesn't make sense the first TileArea may be empty and the second one can't be empty. I find it strange. Moreover, part of the assert and its previous line are kind of redundant.

The assert was added in r18726, where the intersection of TileAreas is used for GetProductionAroundTiles. The TileAreas of industries aren't empty. So the assert would only be triggered if the location of an industry has no tiles (that doesn't happen right now: industries have at least one tile).

Anyway, I would like to intersect TileAreas where one of them may be empty (like the area of a station when its last tile is removed).

@LordAro
Copy link
Member

@LordAro LordAro commented Feb 23, 2019

I'm not a fan of this PR as it stands - it's just a single codechange for no clear benefit.

I'm also not sure when it could trigger either - there was already a check for 0-width TileAreas, and I don't see when a TileArea with height but not width could be used
Personally I think both height and width should have asserts on them - a TileArea with a 0 dimension doesn't make any sense to me at all, kind of like dividing by 0

@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Feb 27, 2019

An empty tile area would usually be indicative of a bug, I think. The real problem would be if you could remote crash a server by sending it a (forged) command that causes it to intersect an empty tile area, I'm not sure if that's possible.

@J0anJosep
Copy link
Contributor Author

@J0anJosep J0anJosep commented Mar 3, 2019

Stupid and non-sense and wrong commit. Closing.

@J0anJosep J0anJosep closed this Mar 3, 2019
@J0anJosep J0anJosep deleted the IntersectionOfEmtpyAreas branch Mar 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants