harbingerofme / DebugToolkit

Debugging commands for Risk of Rain 2. Previously known as RoR2Cheats.
https://thunderstore.io/package/IHarbHD/DebugToolkit/
BSD 3-Clause "New" or "Revised" License
14 stars 8 forks source link

Fix next_boss, Issue #88 #137

Closed SChinchi closed 1 year ago

SChinchi commented 1 year ago

Addresses fixes mentioned in the aformentioned issue.

I partially touched the spawn_ai command as well, since the elite parsing problem also exists there and I took the opportunity to move any argument parsing outside of the loop for spawning monsters.

The SetNextSpawnAsBoss hook may feel a bit inconsistent at the moment, as it is triggered for the teleporter and some of the simulacrum waves. The options are:

Overriding a lunar boss wave to spawn, say, gups, works, but the drop is still linked to the wave controller. I reckon this is a non-issue, as we're only trying to override the boss.


I added the method GetEliteFromPartial, but not its array counterpart. To be frank, this whole GetXFromPartial is starting to smell to me. A lot of code duplication between the two versions and the list_ commands sometimes do the same amount of work, too. I'm starting to think that we only need the array version of GetXFromPartial. If the result is empty, we handle it as an error; no need for unnatural (EliteIndex)(-2) or Equipment.None. Else we either get the first result or all, depending on what we're doing. The list_ commands can then only focus on how to format the fetched results.

This is an issue for a separate PR, but I'm justifying why I didn't add GetElitesFromPartial: it works for the current fix and I expect this will be revisited.

xiaoxiao921 commented 1 year ago

InfiniteTower i'm personally not sure what's the best way to go so I'll let you decide

For the GetXFromPartial I agree if we could reduce code duplication that'd be great though I don't think it's that urgent of a fix

SChinchi commented 1 year ago

InfiniteTower i'm personally not sure what's the best way to go so I'll let you decide

It would make most sense to either keep it strictly for the teleporter, or also support simulacrum. My intuition says that it doesn't hurt having it simulacrum, especially if the docs explicitly also say so. It also ties with the idea that down the line we could add a couple of commands for simulacrum. for example, to set the wave number and select the next wave controller. So I'll go with adding the other hook.

I forgot to ask earlier, but is the log message in the SetNextSpawnAsBoss hook intentionally set to LogLevel.Info?

xiaoxiao921 commented 1 year ago

InfiniteTower i'm personally not sure what's the best way to go so I'll let you decide

It would make most sense to either keep it strictly for the teleporter, or also support simulacrum. My intuition says that it doesn't hurt having it simulacrum, especially if the docs explicitly also say so. It also ties with the idea that down the line we could add a couple of commands for simulacrum. for example, to set the wave number and select the next wave controller. So I'll go with adding the other hook.

Sounds good

I forgot to ask earlier, but is the log message in the SetNextSpawnAsBoss hook intentionally set to LogLevel.Info?

The log is technical so I guess it would make more sense as Debug? It can also serve as a "ok it works" signal for users but yeah probably would make more sense as Debug level

SChinchi commented 1 year ago

There is no Debug level, but it seems Info does that job as unless the log level is elevated, it won't show the message normally.

As far as the changes are concerned, this should be it.