-
Notifications
You must be signed in to change notification settings - Fork 35
Custom level generation #750
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
base: main
Are you sure you want to change the base?
Conversation
|
Was going to test this out but it appears to be under heavy development still (I downloaded Very excited for it to be stable enough to do more stress testing with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code added here is actually in the wrong place, technically. This directory is primarily for Assembly (ASM) Patches
Since this is just function hooks, should probably move this stuff to repentogon/LuaInterfaces/CustomCallbacks.cpp
| return false; | ||
| } | ||
|
|
||
| level->reset_room_list(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it might make sense for the logic to reset & build out the floor be a DungeonGenerator function (like generator->Generate())
May also be good for it to return a success boolean, in case we decide it failed somehow, we could maybe fall back to allowing the vanilla generation to happen again.
| } | ||
|
|
||
| LUA_FUNCTION(place_room) { | ||
| DungeonGenerator* generator = GetDungeonGenerator(L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good for DungeonGenerator itself to contain all its functions such as PlaceRoom (ie, generator->PlaceRoom(config, row, col, seed), and the lua bindings just parse the inputs from lua and redirect to the generator's functions. This will separate the DungeonGenerator logic from the lua bindings, and allow DungeonGenerator code to call its own functions.
| return *lua::GetRawUserdata<DungeonGenerator**>(L, 1, lua::metatables::DungeonGeneratorMT); | ||
| } | ||
|
|
||
| LUA_FUNCTION(place_room) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Use names like Lua_PlaceRoom for the lua bindings, mostly just a consistency thing (not that our consistency is fantastic overall)
| #include "Log.h" | ||
| #include "LuaDungeonGenerator.h" | ||
|
|
||
| DungeonGenerator* GetDungeonGenerator(lua_State* L) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LUALIB_API DungeonGenerator* GetDungeonGenerator(lua_State* L)
I don't know exactly how important the LUALIB_API bit is, but its typically used for such functions. Might impact properly surfacing lua errors.
| }; | ||
|
|
||
| struct DungeonGenerator { | ||
| int a; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a?
| bool is_final_boss = false; | ||
|
|
||
| DungeonGeneratorRoom() { | ||
| this->room = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Prefer nullptr instead of NULL
| DungeonGeneratorRoom generator_room = generator->rooms[i]; | ||
|
|
||
| if (generator_room.room != NULL) { | ||
| LevelGenerator_Room* level_generator_room = new LevelGenerator_Room(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As pointed out by Guantol, this is a memory leak atm
You can initialize a local variable class/struct as just LevelGenerator_Room level_generator_room; and it will be cleaned up when it leaves scope as you'd expect. This also calls the default constructor (for classes at least) automatically where applicable.
Initializing classes with new requires explicit freeing later, though there's usually better options than new
| uint32_t col = (uint32_t)luaL_checkinteger(L, 4); | ||
| uint32_t seed = (uint32_t)luaL_checkinteger(L, 5); | ||
|
|
||
| DungeonGeneratorRoom* generatorRoom = new DungeonGeneratorRoom(config, row, col, seed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This initializes a DungeonGeneratorRoom, and then saves a COPY of it into the array on the next line. The original is also a memory leak.
I think you can initialize it like this:
generator->rooms[generator->num_rooms] = DungeonGeneratorRoom(config, row, col seed);
generator->num_rooms++;
DungeonGeneratorRoom* generatorRoom = &generator->rooms[generator->num_rooms];
This should initialize a DungeonGeneratorRoom inline and assigns it into the array (no copying and no memory management required).
The three example line above would probably be good as a small helper function (ie, DungeonGeneratorRoom* generatorRoom = generator->CreateRoom();?
| DungeonGenerator* generator = GetDungeonGenerator(L); | ||
| RoomConfig_Room* config = lua::GetLuabridgeUserdata<RoomConfig_Room*>(L, 2, lua::Metatables::CONST_ROOM_CONFIG_ROOM, "RoomConfig"); | ||
| uint32_t row = (uint32_t)luaL_checkinteger(L, 3); | ||
| uint32_t col = (uint32_t)luaL_checkinteger(L, 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate the row/col (both bounds checking and verifying that there isn't already a room in that slot)
In theory we'll also need to do other stuff like handling overlapping large rooms and checking door slots, but you don't HAVE to add that right now, unless you want to
|
|
||
| HOOK_METHOD(Level, generate_dungeon, (RNG* rng) -> void) | ||
| { | ||
| bool skip = ProcessGenerateDungeonCallback(this, *rng, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick, but I would suggest replacing the magic numbers for the DungeonType into either an enum or a constexpr, to make the code more readable.
|
|
||
| std::vector<RoomCoords> forbidden_coords = GetForbiddenNeighbors(base_coords, room_shape, doors); | ||
|
|
||
| for (int i = 0; i < this->num_rooms; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest having the generator hold a map that ties gridIdx to roomIdx, in order to quickly find the neighboring rooms, and see if a room already occupies this room's slot, as going through the whole room list is not really efficient.
| return generatorRoom; | ||
| } | ||
|
|
||
| void DungeonGenerator::SetFinalBossRoom(DungeonGeneratorRoom* boss_room) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have the generator hold a single value that defines the index of the final boss room. This would trivially guarantee the constraint that only one room can be the FinalBossRoom, as well as being faster to set and check.
|
|
||
| #pragma endregion | ||
|
|
||
| #pragma region Helpers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest placing these functions inside of their own Utils file, since they can also reused internally and are not strictly for the LuaDungeonGenerator.
| LUA_FUNCTION(Lua_PlaceDefaultStartingRoom) { | ||
| DungeonGenerator* generator = GetDungeonGenerator(L); | ||
|
|
||
| int doors = (int)luaL_optinteger(L, 2, 15); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the start room has essentially a predefined config, and GetRandomRoom can only ever pick one room, due to the variant range being set to just the value 2, there is no need to pass the doors parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doors parameter limits the allowed doors for the room. It could be useful to mimick the behaviour of the mega satan door/polaroid door
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to use a system similar to the blocked grid indices, rather than having the user having to deal with doors directly. In the original level gen the doors parameter is meant as a way to specify which doors are necessary, not those that are allowed. Of course, we should still allow control over the doors parameter, when doing very specific things, like how secret rooms and ultra secret room generation has to mark certain doors as allowed, post layout generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was basing the doors parameter on the LevelGeneratorEntry.doors parameter, since I'm later setting the LevelGeneratorEntry doors field to that. There it does specify the doors argument refers to the allowed doors.
https://repentogon.com/LevelGeneratorEntry.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LevelGeneratorEntry is a wrapper for a LevelGenerator_Room and LevelGenerator_Room only specifies the necessary rooms, not the allowed ones. Most likely the parameter was named like that because the necessary rooms end up being used as the allowed rooms when placing the room.
| } | ||
|
|
||
| HOOK_METHOD(Level, generate_blue_womb, () -> void) { | ||
| bool skip = ProcessGenerateDungeonCallback(this, g_Game->_generationRNG, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I would suggest forcibly making it impossible to generate layouts on dungeon types other than NORMAL, perhaps with an error message explaining that they are currently not supported, since we need to properly evaluate the constraints needed to make the other dungeon types work properly first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to just skip the callback call and write a note in the docs explaining that the callback only works for default generation?
libzhl/functions/Level.zhl
Outdated
| __thiscall void Level::Update(); | ||
|
|
||
| "558bec6aff68????????64a1????????5081ecc0000000a1????????33c58945??535657508d45??64a3????????8bc1": | ||
| __thiscall void Level::reset_room_list(bool unkinitStartRoom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nitpick, change the name of the bool parameter to 'resetLilPortalRoom'
| .push(dungeonType) | ||
| .call(1); | ||
|
|
||
| if (results || !lua_isboolean(L, -1) || !lua_toboolean(L, -1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to change this, as right now I expect that if someone returns an invalid level layout, we don't give a chance for other callbacks to run and simply return to default generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here seems like it might be fine, but yeah this callback will likely require custom handling in main_ex.lua. We could potentially detect that a mod left things in an invalid state, possibly print an error to the console, then reset it before we run the next callback.
Not mandatory for an initial push, but is something we should sort out before this gets included in a release.
| #include "ASMLevel.h" | ||
| #include "../../LuaInterfaces/Level.h" | ||
| #include "../../LuaInterfaces/Room/Room.h" | ||
| #include "../../LuaInterfaces/LuaDungeonGenerator.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding this include is unused, so it should be removed.
| this->shape = room->Shape; | ||
| } | ||
|
|
||
| RoomConfig_Room* DungeonGeneratorRoom::GetRoomConfig(uint32_t seed, int required_doors) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume this function was originally meant to do something else due to the unused parameters and unnecessary check for nullptr, what was this supposed to do originally?
| .push(dungeonType) | ||
| .call(1); | ||
|
|
||
| if (results || !lua_isboolean(L, -1) || !lua_toboolean(L, -1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here seems like it might be fine, but yeah this callback will likely require custom handling in main_ex.lua. We could potentially detect that a mod left things in an invalid state, possibly print an error to the console, then reset it before we run the next callback.
Not mandatory for an initial push, but is something we should sort out before this gets included in a release.
| this->level_generator._isXL = false; | ||
| } | ||
|
|
||
| bool DungeonGenerator::CanRoomBePlaced(XY& base_coords, int shape, int allowed_doors, bool allow_unconnected) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When placing rooms with RoomConfig_Rooms, we may need to validate somewhere that the two rooms are actually able to mutually connect allowed doorslots to each other. I dont think vanilla really does this since iirc it specifically queries rooms with specific doors available when filling the layout with configs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already handled by the allowed_doors parameter. When a room is placed in the layout it blocks all inaccessible grid indexes
| 2, | ||
| 0, | ||
| 10, | ||
| (unsigned int*)&required_doors, // If I don't do it like this it shits itself |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just make the int required_doors variable an unsigned int to begin with so you dont have to do the weird casting
| return 1; | ||
| } | ||
| else { | ||
| return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In cases like this, instead of returning 0:
lua_pushnil(L);
return 1;
Prefer to only return 0 for functions that never return anything.
Dunno how severely this matters, but it seems like the consistent thing to do
Added a new callback and a couple functions that allow for mods to generate custom floors.
MC_PRE_GENERATE_DUNGEON
Called before
level::generate_dungeonis called, which places all the rooms in a floor. Passes an RNG.Return true to cancel vanilla floor generation.
Level:ResetRoomList(bool)
Needs to be called before placing any rooms in the custom level.
The bool is the same as the one in
level::init. Testing it so far, setting it to true makes it work all the time.Level:SetLastBossRoomListIndex(int)
Also needs to be called after creating the custom floor, since not setting it crashes the game when continuing.
Doesn't actually need to be set to a boss room for the game to not crash, but I haven't tested any other side effects apart from boss rooms not spawning the trapdoor if they are not the last.
Sample
