Skip to content
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

Reimplement KaleidoScope_DrawItemSelect, Restore Equip Swap #2115

Merged
merged 18 commits into from
Apr 22, 2024

Conversation

flagrama
Copy link

This needs testing and some more cleanup, but this replaces the ASM hacks for free movement on the item pause screen by reimplementing KaleidoScope_DrawItemSelect in C. This gives more flexibility in changing behaviours in this menu.

As well as reimplementing the function, the glitch known as Equip Swap has been restored by limiting the exact range of freedom originally given to the Vertical movement in the menu. You can no longer select empty slots on the far left or far right by pressing up or down. The behaviour if there are items above or below with empty spaces should be unchanged though. This makes menuing slightly more awkward, but not massively. It also still prevents any potential softlocks from item placement since the central area is completely free to move around in, and the Horizontal axis has no limits at all.

Some additional hacks had to be reimplemented, but they were pretty simple to add in to the C code. Code to prevent you from equipping an empty slot has been reimplemented, and the clever hack to prevent the "equip to c buttons" textures from appearing on empty slots was reimplemented as well.

I tried to leave plenty of documentation in the code, at least as to how far I understand it after hacking away on it, as well as mentioning all of the hacks that randomizer required that a plain reimplementation would not have needed.

@fenhl fenhl added Type: Enhancement New feature or request Component: ASM/C Changes some internals of the ASM/C libraries labels Oct 14, 2023
Copy link

@AngheloAlf AngheloAlf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could be a good idea to add a plando test file with a seed that starts with the problematic cases for equip swap, so it is easier for people to test this locally

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there are plans on adding a linker script to manually specify sections I suggest this linker script to be named something like oot_symbols.ld, original_symbols.ld, rom_symbols.ld, undefined_syms.ld, etc

(linkers allow passing multiple linker scripts on the invocation.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also suggest adding a note like "This symbol list is ordered by the address", to remind future contributors to keep the list ordered

Copy link
Author

@flagrama flagrama Oct 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about including an entire folder of *.ld files. I kind of would like to have the sfx stuff in an audio file, the dl42 setup and texture/icons in a graphics file, and the rest in a kaleido file. It might seem like overkill at this point, but looking at z64.h this file is bound to get out of hand quickly even if it is just existing symbols. (this question isn't exactly directed at you, should be easy enough to include multiple linker files in the makefile)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having specialized linker script files sounds like a good idea to me. Just take in mind that you will need to pass the -T flag for each one of them (-T file1.ld -T file2.ld -T file3.ld)

@flagrama
Copy link
Author

I think it could be a good idea to add a plando test file with a seed that starts with the problematic cases for equip swap, so it is easier for people to test this locally

I already shared patches with a known currently problematic setup in the discord dev channel to have some people test them and have tested them myself now. At this point if someone wanted to test something they'd know better than me what needs tested.

@flagrama flagrama marked this pull request as ready for review October 16, 2023 02:27

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking again about this. I'm not that sure having multiple linker scripts for defining symbols is that good. Sounds annoying to maintain for the rando people in the long run

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anyone else have opinions on this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably prefer the single file as well

@cjohnson57 cjohnson57 added the Status: Waiting for Release This PR is ready for merge, but we're holding off on it until after the next release label Nov 11, 2023
@cjohnson57 cjohnson57 added Status: Needs Review Someone should be looking at it Status: Needs Testing Probably should be tested and removed Status: Waiting for Release This PR is ready for merge, but we're holding off on it until after the next release labels Nov 11, 2023
@cjohnson57 cjohnson57 removed the Status: Needs Testing Probably should be tested label Nov 29, 2023
# Conflicts:
#	ASM/Makefile
#	ASM/build/asm_symbols.txt
#	ASM/build/bundle.o
#	ASM/build/c_symbols.txt
#	ASM/c/item_upgrades.c
#	ASM/c/z64.h
#	data/generated/rom_patch.txt
#	data/generated/symbols.json
@cjohnson57 cjohnson57 merged commit c249260 into OoTRandomizer:Dev Apr 22, 2024
3 checks passed
@fenhl fenhl added this to the next milestone Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ASM/C Changes some internals of the ASM/C libraries Status: Needs Review Someone should be looking at it Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants