Repository navigation
TileGrid public Tile type - #1138
TheDavSmasher wants to merge 15 commits into
Conversation
maddie480-bot
added
the
1: review needed
SnipUndercover
added
the
4: should be squashed
|
victory |
Wartori54
left a comment
Wartori54
left a comment
There was a problem hiding this comment.
The idea sounds good but (apart from the few comments I made) theres a major issue (related to the TileIds comment).
Mods also modify the values of TileGrid.Tiles opening up the possibility of TileIds to getting desynced, examples that I found are: KoseiHelper, FancyTileEntities, ScugHelper... and many others likely.
I feel like this still is an issue that should get addressed, even if the current impl covers 99% of cases, because users of the new API will get very bizarre bugs once something goes wrong.
The two solutions that come to mind for me are:
- Use relink patches to find mods modifying
TileGrid.Tiles, and insert the appropriate code besides it, OR use relink patches to change theTileGrid.Tilesfrom aTileGridto a new type were we manage both the tilesVirtualMap<MTexture>and theVirtualMap<char>. - Introduce a dirty flag inside
VirtualMap<T>for each element so we can catch assignments.
(Or just ask all modders to fix their code. :) )
But any other suggestion is welcome too.
| namespace Monocle { | ||
| class patch_TileGrid : TileGrid { | ||
|
|
||
| public VirtualMap<char> TileIds; |
There was a problem hiding this comment.
In TileGrid there are several methods that mutate the vanilla VirtualMap<MTexture> Tiles which should also get appropriate handling.
Unfortunately, those are hard to implement since there's no id we can trivially use, and getting proper values would imply creating a very ugly Dictionary<MTexture, char> or something of the sort. So I suggest adding an unknown value to the TileIds for when an id was lost.
Edit: Just realized that Celeste.TileInterceptor would need the same handling too.
| public char TilesetIdAt(Vector2 readPosition) { | ||
| Vector2 position = Entity.Position + Position; | ||
| int num = (int) ((readPosition.X - position.X) / 8f); | ||
| int num2 = (int) ((readPosition.Y - position.Y) / 8f); | ||
| if (num >= 0 && num2 >= 0 && num < TilesX && num2 < TilesY) { | ||
| return TileIds[num, num2]; | ||
| } | ||
| return default; | ||
| } |
There was a problem hiding this comment.
readPosition being a global position feels weird here, do you have any argument on why this should not be relative to the entity position?
There was a problem hiding this comment.
Not really, besides utility and code I based this on, but I can make an overload/alternative that doesn't utilize global position and instead uses room position?
maddie480-bot
added
2: changes requested
This PR modifies the Autotiler/Tiles and TileGrid classes so TileGrid inherently keeps a reference to the tileset id that was used to fill that portion of the grid, as well as a method to get an id at a position from that grid. This is done in an effort to access the specific tileset on contact for further optionability based on tileset id instead of only via sound index.