feat: add doors during level generation #39
Conversation
| /// <summary> | ||
| /// The size of the tiles in the x-direction. | ||
| /// </summary> |
There was a problem hiding this comment.
Could you please add the s_ prefix to the constants below? ;)
There was a problem hiding this comment.
I think our wiki actually says it should be PascalCase, but the editorconfig says it should be _camelCase
| public int doorId; | ||
| /// <summary>the door object this script is attached to</summary> | ||
| [SerializeField] private GameObject parent; | ||
| [SerializeField] private int _doorId; |
There was a problem hiding this comment.
I wouldn't make this private, because it needs to be accessed to check whether or not the player has the right key
There was a problem hiding this comment.
The door ID could still be private, and a method can be added to generate a key for the given door, and another method to check if the given key is correct. This could all be 'internal' logic, because it is not relevant to the player how this check is performed.
There was a problem hiding this comment.
I would argue that the field shouldn't even be serialized.
There was a problem hiding this comment.
But why would you do it like that, adding so much complexity to a simple object like a door seems redundant. Just give the door a number than can be checked
There was a problem hiding this comment.
- I don't feel like it adds complexity. The logic has to be implemented somewhere; doing it here takes away complexity from other classes (otherwise the same logic would have to be implemented in some player or key script).
- When you envision a door, you don't care about some ID it has. Doors in real life also don't have IDs.
- Deciding whether or not the key is correct for the door is not the responsibility of the player or the key; only the door should be able to decide if it can be opened. The key is just a tool to aid that decision and the player will have to oblige to the door's rules. In real life, you also wouldn't be able to open the door anyways through some hack, but that would be possible if you make the player responsible for deciding whether or not the door is actually locked.
- Imagine this scenario; another type of door is added later that adds more flexibility in what keys it allows to be opened with (maybe any key is sufficient for example?). If you implemented this logic in some player class, the player class would have to account for that new door type as well. On the other hand, if it is implemented in a door script, then the player doesn't have to care one bit about this change. The player only cares if the door is open after supplying some key. Or maybe the key must be of some special type and the ID is not relevant at all... then the player can still just supply a key and see if the door is opened after, not worrying about any of the logic.
There was a problem hiding this comment.
- that's a fair point, but I think it's equally as logical to implement it somewhere else, the door, the key, and the player turning the key have equal impact when opening a door
- The ID is a representation of the notches on the key/inside the lock so in a way it does
- This is one solution, but again I think the door is not more responsible for deciding if it opens than the key or player
- As long as the door has a function to be removed, I don't think it makes it more difficult at all to open it in other ways. If I were to make an item that opens the next 3 doors, I would create the logic there and just call the destroydoor function where needed
There was a problem hiding this comment.
- They might have equal impact in the sense that they all play a role in opening a door. However, they don't have equal responsibilities.
- Correct; "inside the lock". A real-life door uses an 'ID' in the same way a virtual door should: for the inner logic. As a frequent door opener myself, I can say I have never seen the insides of a lock. I'm a simple man: I see a keyhole, I put my key in it, and the door opens. So
privateseems the desirable accessibility for the ID. - Of course the exact responsibility is debatable, but consider real life again. You cannot move through a door if the lock is broken. The key, or me as a door opener, cannot decide to 'open the door anyway', despite the broken lock. A broken key, however, is no problem in most cases. Just grab another key. Or we do not have another key; pity, but then we just have to accept that we cannot open the door. We have no power to open the door at that point. But the door is still working as it should, waiting for the right key.
- It doesn't necessarily make it more difficult to open the door in other ways. In fact, I think it makes it easier to open the door in other ways if you give the door a public function to be removed. However, I'm convinced this will cause tightly coupled code, illogical code architecture, etc., giving rise to bugs, increasing the difficulty to implement new features, and overall just making the code harder to read and understand.
There was a problem hiding this comment.
I agree with @JensSteenmetz on this. I will remove the SerializeField attribute.
| Vector3 roomPosition = new(x * _tileSizeX, 0, y * _tileSizeY); | ||
| Quaternion roomRotation = Quaternion.Euler(0, room.Rotation * _tileRotation, 0); | ||
| (Vector3 relativeDoorPosition, Quaternion relativeDoorRotation) = direction switch | ||
| { | ||
| 0 => (new Vector3(-_tileSizeX / 2f, 0, 0), Quaternion.Euler(0, -_tileRotation, 0)), | ||
| 1 => (new Vector3(0, 0, _tileSizeY / 2f), Quaternion.identity), | ||
| 2 => (new Vector3(_tileSizeX / 2f, 0, 0), Quaternion.Euler(0, _tileRotation, 0)), | ||
| 3 => (new Vector3(0, 0, -_tileSizeY / 2f), Quaternion.Euler(0, 2f * _tileRotation, 0)), | ||
| _ => throw new UnityException("Invalid direction when placing door") | ||
| }; |
There was a problem hiding this comment.
I found this a little bit hard to read (maybe it's just me, but I wouldn't mind some extra comments here)
| Vector3 roomPosition = new(x * _tileSizeX, 0, y * _tileSizeY); | ||
| Quaternion roomRotation = Quaternion.Euler(0, room.Rotation * _tileRotation, 0); | ||
| (Vector3 relativeDoorPosition, Quaternion relativeDoorRotation) = direction switch | ||
| { | ||
| 0 => (new Vector3(-_tileSizeX / 2f, 0, 0), Quaternion.Euler(0, -_tileRotation, 0)), | ||
| 1 => (new Vector3(0, 0, _tileSizeY / 2f), Quaternion.identity), | ||
| 2 => (new Vector3(_tileSizeX / 2f, 0, 0), Quaternion.Euler(0, _tileRotation, 0)), | ||
| 3 => (new Vector3(0, 0, -_tileSizeY / 2f), Quaternion.Euler(0, 2f * _tileRotation, 0)), | ||
| _ => throw new UnityException("Invalid direction when placing door") | ||
| }; |
| Corner _ => _roomObjects.Corner, | ||
| Crossing _ => _roomObjects.Crossing, | ||
| DeadEnd _ => _roomObjects.DeadEnd, | ||
| Empty _ => _roomObjects.Empty, | ||
| Room _ => _roomObjects.Room, | ||
| Straight _ => _roomObjects.Straight, | ||
| TSection _ => _roomObjects.TSection, | ||
| _ => throw new UnityException("Unknown tile type when placing tile") | ||
| }; |
There was a problem hiding this comment.
I would argue the _'s can be removed.
|
Btw, this branch is named incorrectly. It should start with |
That last part is not true, see the wiki and other branches. |
|
This PR is closed now because I changed the branch name. If you have a local copy of the branch please run: git branch -m 68-spawn-doors feat/68-spawn-doors
git fetch origin
git branch -u origin/feat/68-spawn-doors feat/68-spawn-doors
git remote set-head origin -a |
Description
This PR adds automatic placement of doors during level generation. The doors are placed when a room is placed, based on the available directions of that room. Each door gets a unique ID which we will use in the future to match doors with keys.
Testability
There is no level generation yet, but there is a hard-coded level in the
TempFillFunctionin GridPlacer.cs.You can play this level in the GridSystem scene. The rooms should have the correct doors placed according to the hard-coded values in
TempFillFunction. The doors should open when approached by the player.Checklist