klangner / mapgen.rs

Map generator for games.
https://klangner.github.io/mapgen.rs/
Apache License 2.0
44 stars 5 forks source link

Replace instances of `TileType::Wall` and `TileType::Floor` with `TileType::Blocked(0)` and `TileType::Unblocked(0)`, respectively. #36

Closed ndarilek closed 3 years ago

ndarilek commented 3 years ago

Wait, I thought that was the entire idea of this issue, and you seemed to approve initially.

It's your project, so either way. But I read the book that you based this project on, and it doesn't get far at all before it starts creating maps for towns with peers, forests with trees, etc. And I get why you don't want to add a bunch of custom tile types so this library has peer and tree tiles, for instance. But if I wanted to follow this tutorial and build a forest map using a filter compatible with your crate, your design won't let me. If I want to start with a filled map and apply various filters, then convert remaining tiles to trees in some pattern, there's no way I can do that with an architecture whose entirety is walls and floors.

I'd like to collaborate, but will have to fork and go my own way if something like this doesn't land. Again, I don't want to expand it beyond this, but I just don't see the usefulness of an architecture generic enough to allow filters but not generic enough to at least have more than two tile types.

klangner commented 3 years ago

Yes you right. I agree that there is a need for more information about tiles then Blocked/Unblocked. But personality I would like to make it different. For example when I look at the code:

if map.at(x as usize, y as usize) != TileType::Blocked(0) {                        
    can_build = false;                    
}

This is wrong. This should match blocked no matter the value. It is confusing. Is Blocked(1) also blocking or not? Thats why I would prefer to move this value to separate vector like TileObject. Ideally I would also like to use typed TileObject. No ints. So instead of inserting '5' I would like to say: TileObject::Water. But that would probably require parametrised Map, since there could be different TileObjects for different maps. Another potential solution is to create trait for Tile with functions like is_blocked() and type() and use it instead of TileType.

If you want to resign from this value, that would be great. I will try to go back to this project to add TileObject, since I agree we need it. If for some reason you decide to fork it, that's ok too :-) It is open source for this reason. I will try to solve the problem with TileObject and maybe then convince you that it is better this way :-)

And no matter what I appreciated your work.

ndarilek commented 3 years ago

OK, that's a fair point. You're right in that Blocked(1) should be blocking. I'll do a check and match against all blocking values.

I'm also not sure how to move toward a more TileObject-like design without adding lots of game-specific values. There do need to be more tile type possibilities, but I'd really like to avoid adding Water, Dirt, Grass, etc. because that may not fit the style of many games. If the goal is to keep the structure game-agnostic, then the library only cares about two things IMO. 1. Is this tile blocked so I shouldn't build/modify it, or is it empty and open for modification? 2. For custom filters implemented in games, is this tile type something another custom filter might care about? To that end, I tried to capture both concepts in the simplest way possible.

I'm open to other alternatives, and apologize for the misunderstanding. I'll go through the previous patch and make sure we're reacting correctly to all variations of blocked/unblocked tiles.

klangner commented 3 years ago

Yes you are right, that adding specific tiles like Water is the wrong idea. Still some generators may need to add more information then Blocked/Unblocked. I'm not sure how to solve it yet. I have some ideas but I need to check them first. for example maybe create tile as:

struct Tile {
 is_blocekd: bool,
 type: usize,
}

I'm still not happy with this untyped usize, but this can be changed later, since I'm not sure what to do about it.

BTW I think you understood this correctly. It is my fault as I was not paying enough attention to this problem when we discussed it in the issue. Do you want to give this new struct a try?

klangner commented 3 years ago

Hi, I have implemented this feature using struct for Tile. Let me know if it is enough for you. I will also close this PR as it is no longer relevant. Thank you!