Time independent skulltulas - #2332
Conversation
|
I guess for logic you'd go around to each skulltula you affected that checks for night and change at_night to (at_night or [setting]). Something to look out for is the watchtower, which will no longer be climbable in child day without a means of killing the skulltula. In closed forest, the time of day access check is skipped, so there's special logic on the night skull there to confirm you can actually get it to night somehow, and you'd also have to add the new setting as an alternative for that. |
40b8106 to
4c1fc19
Compare
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[0].headers[0].actor_list.actors[4], | ||
| # GTG Stalfos Room | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[1].headers[0].actor_list.actors[2], | ||
| # GTG Flame Wall Maze | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[2].headers[0].actor_list.actors[15], | ||
| # GTG Pushblock Room | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[3].headers[0].actor_list.actors[10], | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[3].headers[0].actor_list.actors[11], | ||
| # GTG Rotating Statue Room | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[4].headers[0].actor_list.actors[3], | ||
| # GTG Megaton Statue Room | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[5].headers[0].actor_list.actors[18], | ||
| # GTG Lava Room | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[6].headers[0].actor_list.actors[17], | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[6].headers[0].actor_list.actors[18], | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[6].headers[0].actor_list.actors[19], | ||
| # GTG Dinolfos Room | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[7].headers[0].actor_list.actors[14], | ||
| # GTG Ice Arrow Room | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[8].headers[0].actor_list.actors[4], | ||
| # GTG Shellblade Room | ||
| scenes[SceneIDs.GERUDO_TRAINING_GROUND].rooms[9].headers[0].actor_list.actors[22], | ||
| # Death Mountain Crater | ||
| scenes[SceneIDs.DEATH_MOUNTAIN_CRATER].rooms[1].headers[2].actor_list.actors[44], | ||
| # Thieves' Hideout Green Cell Room 3 torches | ||
| scenes[SceneIDs.THIEVES_HIDEOUT].rooms[1].headers[0].actor_list.actors[9], | ||
| # Thieves' Hideout Red Cell Room 1 torch | ||
| scenes[SceneIDs.THIEVES_HIDEOUT].rooms[2].headers[0].actor_list.actors[9], | ||
| # Thieves' Hideout Green Cell Room 4 torches | ||
| scenes[SceneIDs.THIEVES_HIDEOUT].rooms[4].headers[0].actor_list.actors[11], | ||
| # Thieves' Hideout Blue Cell Room 2 torches | ||
| scenes[SceneIDs.THIEVES_HIDEOUT].rooms[5].headers[0].actor_list.actors[14], | ||
| ] | ||
| for address in wonder_talk2_y_coordinates: | ||
| rom.write_byte(address, 0xFB) | ||
|
|
||
| # Pre-scene-framework hack replaced the first byte with 0xFB | ||
| bit_mask1 = int.from_bytes(b'\xFB\x00', 'big', signed=True) | ||
| bit_mask2 = int.from_bytes(b'\xFB\xFF', 'big', signed=True) | ||
| for actor in wonder_talk2_actor_entries: | ||
| actor.pos.y = (actor.pos.y | bit_mask1) & bit_mask2 | ||
|
|
||
| if 'frogs2' in settings.misc_hints: | ||
| # Prevent setting the replaced textbox flag so that the hint is easily repeatible by walking over the spot again. | ||
| # And move the hint spot down the log so that it doesn't pop every time a song is played, and let some room to do ocarina item glitch. | ||
| rom.write_int16s(0x2059412, [0x03C0, 0x00E2, 0xFAA6]) # Move coordinates. Original value : 1000, 205, -1202. New value : 960, 226, -1370. | ||
| rom.write_byte(0x205941F, 0xBF) # Never set the flag. | ||
| actor = scenes[SceneIDs.ZORAS_RIVER].rooms[0].headers[0].actor_list.actors[58] | ||
| actor.pos = Vec3s(960, 226, -1370) # original 1000, -205, -1202 | ||
| actor.params = 0x4BBF # original 0x4BBB | ||
|
|
||
|
|
||
| # Gaps in IDs are intentional | ||
| # https://github.com/zeldaret/oot/blob/7235af2249843fb68740111b70089bad827a4730/include/z64cutscene.h#L35-L165 | ||
| class CutsceneCommandID(IntEnum): |
There was a problem hiding this comment.
man, rando sure is a hot mess when it comes to organization.
I find it kind of odd to just stuff these important type definitions after a bunch of function patching routines split out of patches.py. I think we need to consider moving this type access stuff into a new folder like "ztypes" or something
There was a problem hiding this comment.
You're right, I should have put more effort into structure. Some of the other utility classes like Vec3s and Vec3i that didn't fit well got put into Rom.py despite not fitting there either. I can refactor into the same file/folder structure as decomp for structs lifted from there, with a top level folder to keep it neat.
|
I warned you about special logic that would be needed for KF GS Know It All House! As you have it currently, in closed forest, if you start at daytime, under this new setting the skulltula will be there but you won't have logic to collect it before beating deku or finding sun's song. That skull has an extra check on it to make sure it's actually present since closed forest breaks the assumption that you will always have access to moving time of day from spawn, so you need to add the new setting as an alternative way to have the time of day to access it. (This isn't really related to your PR, you shouldn't change anything about this, but I wonder if all of those skulltulas that check for ToD in areas where time moves already, should just have the time of day check removed. It was nice to look at before but with the new setting it's become kind of bloated. I made a choice along those lines with Peahats when reviewing enemy drop shuffle b/c separate night and day logic for them was pretty convoluted for no real benefit.) I don't know enough about glitched to say whether or not the logic is wrong absolutely for sure or not, but at a glance I don't trust it. For example, man on roof, you check for a weapon to kill the skulltula, but you always are checking it regardless of if the skulltula is potentially there at daytime. You should get a glitched logic expert to review it, or make no glitched logic changes and mark the settings as incompatible. (Unrelated to your PR but I was glancing over the night skulls in glitched logic and noticed some logic for buying the Giant's Knife that is assuming that the Knife isn't shuffled. This shows the dangers of marking new settings as glitched logic compatible without getting a specialist's review!) |
|
Should this setting be enabled in the Easy Mode preset? |
|
It would make sense. Would that need to go through some sort of vote or poll? |
|
Current dev team call is in favor so I'd say you can just add it. |
|
@r0bd0g >
I wouldnt worry too much about glitched logic right now, because it's being updated entirely to have all settings included. There are way more important stuff that isnt compatible at all currently and in released versions. If this is going to be slated for 9.0 as well then we can make it incorporated. I dont know the timing for this PR. |
|
There's not really any timing for this PR, it will be merged once it's been code reviewed and sufficiently tested. Since it's a rather large change, that might take a while, but so is glitched logic 3.0, so it's hard to say which is more likely to be merged first. I'd recommend not including the changes for this in glitched logic 3.0 for now or perhaps maintaining them separately. |
wasn't meaning that it would wait for it but that I'd incorporate it into logic when the time comes. Even if it's after being pulled. |
|
I'll see if we can get a web generator for this branch set up to encourage more testing. |
|
FYI, I don't have much free time through at least fall, possibly into next year. I'm still around, but changes might be delayed. |
30876af to
1af8bc0
Compare
cjohnson57
left a comment
There was a problem hiding this comment.
No real blockers, just a couple small things I noticed. Though, I feel a little impotent in my ability to review this properly, given the scope and the fact that there's thousands of lines of transliterated game data that I can't really have an opinion on.
I kind of wonder if it's worth removing the nighttime skulls expect sun song option now... It feels a bit vestigial. Doesn't do the same thing obviously, but fulfills the role of "I don't want to mess with waiting for night for skulls)
Also, I see this is still tagged as needs testing, is this actually still the case?
|
|
||
| # Jabu Jabu swallowing Link | ||
| patch_cutscene_destination_and_length(rom, 0xCA0784, 1) | ||
| jabu_swallow_cutscene = Cutscene.decode(rom, 0xC9FC84) |
There was a problem hiding this comment.
Not something that needs to be fixed really but it would be nice if these "magic numbers" could be stored in a table too, like how many of the others have been eliminated by these changes
There was a problem hiding this comment.
Agreed it could be better documented. Cutscenes are stored all over the place in the codebase with no definitive centralized list unfortunately. This one is in the Jabu actor file for example and only called by it.
The cutscene objects try to minimize the magic VROM locations to just the start of the cutscene. Those are easy to line up with the decomp ntsc-1.0 map file for the cutscene variable address. In the future, maybe there could be a writeup at the top of the file on how to do that.
| def read_s8(self, address: Optional[int] = None) -> int: | ||
| if address is None: | ||
| address = self.last_address | ||
| return int.from_bytes(self.read_bytes(address, 1), 'big', signed=True) | ||
|
|
||
| def read_s16(self, address: Optional[int] = None) -> int: | ||
| if address is None: | ||
| address = self.last_address | ||
| return int.from_bytes(self.read_bytes(address, 2), 'big', signed=True) | ||
|
|
||
| def read_s32(self, address: Optional[int] = None) -> int: | ||
| if address is None: | ||
| address = self.last_address | ||
| return int.from_bytes(self.read_bytes(address, 4), 'big', signed=True) |
There was a problem hiding this comment.
Is there a particular reason not to make these one function with the size as a param? (Though that being said, I think I might have done the same thing in my model code)
There was a problem hiding this comment.
This is pretty standard when working with binary data. Having to specify the size as a parameter just makes each call harder to read because you have to think about which one is the size parameter and may have to refer to the function definition, but by having separate calls for each size makes it obvious. Especially with these being for signed bytes I think it is very important to make it absolutely obvious what the function is doing.
There was a problem hiding this comment.
Basically what flag said. With how often these are called in the parsing functions, it's more ergonomic to do rom.read_s16(address) than something like rom.read_bytes_to_int(address, length, signed=True). Also makes it more obvious in the calling code what the C type of the data is supposed to be.
No, I consider this to be sufficiently tested after the recent full-game test. I guess I forgot to remove the label. |
To be clear, I've had multiple full-game tests of this on console and emulator for a while in addition to all the individual cases checked off in the PR description. Always preferable to have independent testing though. |
|
…I was not aware of that. |
|
I'm not sure I ever said that to be fair, looking back at the description. |
|
Oh well, like you said, doesn't hurt to have independent confirmation too. |
Implements #2356.
Two parts to this PR. There is the skulltula time of day feature, and a new framework to enable arbitrary changes to any of the scene or room files parsed to linked python objects. The framework automatically shifts all pointers within the files to handle new file locations and additions/deletions within the file. It will have multiple applications besides the skulltula feature:
More details on the design of the parser is included in
Notes/scene-and-room-patching.md.GUI Impacts for Web
TL;DR,
Creating Patch Fileis added as a progress bar milestone betweenPatching ROMandStarting compression.Due to the increased time spent patching the rom caused by the significantly larger amount of changed bytes compared to the current randomizer, an additional status message milestone is added to the GUI progress bar:
Creating Patch File. This was previously included in thePatching ROMmilestone. Additionally, as these milestones alternate for each player in multiworld generation, the progress percentages for these two milestones were modified from a static number to one that scales based on the currently processing world. This requires resettingcurrentWorldafter successful patching for subsequent generations to work whenGeneration Countis greater than 1.Python Virtual Environment Addition
This PR makes use of the numpy package to more efficiently process changed bytes for patch generation compared to base python lists and dictionaries. While the randomizer has avoided external dependencies thus far, using numpy reduces the performance penalty substantially (times are from single world runs on an AMD 5900X with the S8 Tournament preset):
A virtual environment is automatically created and activated by the major entry points to the randomizer:
GUI.pyOoTRandomizer.pyCI.pyEach of these entry points independently check if they are running in a virtual environment located in subfolder
.venvat the local path. If the environment doesn't exist, it is created automatically. There is also check if all dependencies inrequirements.txtare fulfilled via pip. If not fulfilled, they are installed automatically in the virtual environment. If running in a release build, any pip errors are captured and passed to the regular error popup. Otherwise, running via the scripts logs errors to the console before exiting. If venv and pip checks are successful, but either the current python environment is not the virtual environment, or new dependencies were added, the script relaunches itself via thesubprocessmodule in the virtual environment with all arguments intact. This behavior can be added to additional entry points or custom scripts by importing theensure_venv()function fromUtils.pyand adding it as the first line wherever it's needed. Seegui_main(),start(), and the top level ofCI.pyfor example usage.numpy is pinned between the last version supported by python 3.8 (1.24.4) and the version in use on RealRob's
custom_voice_packsbranch (2.2.1). Both versions have been tested to work. For compatibility with RealRob's work, I strongly recommend increasing the minimum python version for the randomizer to 3.10, the earliest supported by numpy 2.2.1. 3.8 is EOL, and 3.9 will be EOL in October 2025. This PR does not change the minimum python version.Unrelated Bugfixes
OoT-Randomizer/Main.py
Line 267 in a821705
Rom.copy()handling of theoriginalproperty. On copy, the new object'soriginalwould refer to itself instead of the unmodified rom buffer. This was causing problems re-using the patched rom before cosmetics to output a rom and zpf at the same time. This isn't a problem with the current randomizer as it always fully repatches the rom for every output. The same issue also applied to theoverlay_table, but that had no ill effects.Todo
DMA changes for compressor?Rando Patches to Test with New System
Well ladder rupee repositionNo longer relevant as of Fix for ladder cutscene softlock vanilla bug #2551