RogueSharp

RogueSharp copied to clipboard

Currently the GetCell method of Map returns a concrete Cell class. While you can inherit Map and implement your own features, there's no way to stuff your implementation of the ICell interface into your implementation of Map as the GetCell method always returns a concrete type of Cell.

To be able to have your own ICell implementations maybe Map needs a generic <T> or something (similar to IMapCreationStrategy) or GetCell should be virtual with GetCell<T> where T is ICell.

I second this. While the library has been almost perfect for me (great job!), I'm having issues inheriting from map with my own cell/map features. At the very least GetCell would need to be virtual and some private members made protected.

Ah nm, I should have looked at the latest commits. Looks like you're already on it.

Ah nm, I should have looked at the latest commits. Looks like you're already on it.

If you get a chance to try that branch https://github.com/FaronBracy/RogueSharp/tree/InheritableMap I'd be curious to know how it works for you. I wanted to try it out in a few samples before merging it in to make sure it would work well for people.

I've tried it out and it's working perfectly - really great design. I inherited from both Cell & Map and successfully created four new map creation strategies inheriting from IMapCreationStrategy<TMap, TCell> (ie. Drunken Walk, Forest w/Lake, Maze, and BSP Grid Rooms). The ability to inherit from Cell & Map now and extend it with my own data & logic is exactly what I needed.

I think the only thing I would request is possibly making _fieldOfView & _cells protected rather than private so that the sub class can use them directly. Either that or if you want more hiding, create protected accessor methods for them. Right now I don't think there's a way to get at the raw FieldOfView object from the sub class (though I haven't really needed it, so not an issue). And more importantly, the only way to get at all the cells would be GetAllCells, which recreates the enumerable every time and is really unneeded (performance-wise) inside the sub class. But that's totally not a big deal, functionally it's all working perfectly.

Thanks again for such a great library!

There are already a lot of methods in the Map class to access cells including getting all cells or an individual. I don't think it would be good to expose _cells as protected as it would almost negate a lot of those access functions. Instead if there's a particular need to access a cell or cells you can't get from the current implementation I would think a PR with a new method would be prudent. Same for _fieldOfView, although there are not a lot of functions for this but not sure what your specific needs are. I think just making these fields protected is a somewhat lazy solution but maybe that's just me.

Yeah I see that there are already ways to get at all the cells ... simply calling GetAllCells is sufficient. I can see this both ways. However, performance-wise, is it necessary to iterate and create a new enumeration every time you want to access it from a sub class? I get hiding it from outside the classes, but that's the whole purpose of protected is to expose the raw data to the sub classes for immediate use. I'm not thinking of lazy implementation, I'm thinking of performance. Why bother consistently cycling through and recreating the enumeration, when the grid of cells is right there inside the class that I'm inheriting from and enclosing.

But again, it's not that big of a deal. Even for performance, it's not like a roguelike is killing the machine and this was only important during map generation which is isolated anyway :-)

Agreed and it's up to @FaronBracy how he wants to implement it. Not sure what performance issue there is accessing cells through the various functions since everything ultimately calls GetCell. There's no boxing/unboxing I know of around creation of an IEnumerable, but I could be wrong. The advantage of accessor functions is not just hiding but encapsulation of all that you need to do before handing back a value. For example the implementation of GetCell doesn't just return _cells[x,y] it also sets the cells IsInFov using the _fieldOfView, so you get the latest information at runtime you might need, making further calls unnecessary. In any case whatever works but the base implementation works great so I would consider this a merge candidate.

Agreed and good points. I was hoping for quick access to the raw cell grid because I was unconcerned with field of view during map generation (so why call GetCell 14,400 times on a 120x120 grid if not needed). However, you're right that it does lead to an inconsistency that probably should be avoided, performance really isn't an issue, and the base implementation does work great as is, so I would definitely consider this a merge candidate as well.

Thanks again for trying out the changes. I've merged these in https://github.com/FaronBracy/RogueSharp/pull/31.

There is also a new pre-release NuGet package here. https://www.nuget.org/packages/RogueSharp/5.0.0-pre2