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

Replace Magic Meter Items #2081

Open
wants to merge 1 commit into
base: Dev
Choose a base branch
from

Conversation

Maplesstar
Copy link

Replaces both the base and double magic items with a bar. Color matches cosmetics.
Doing this to free up the regular magic models for randomizing magic refills as junk items in the pool in the future.
Chose to do the same model for both base and double magic because it's a bar, getting a second and sticking it to the first makes sense to me and making a longer one isn't really feasible without allowing it to poke out of it's space, like the box in Graveyard.

image
image
image

Replaces both the base and double magic items with a bar. Color matches cosmetics.
@Natheirean
Copy link

This is just a personal opinion from a runner's perspective, but I don't like the idea of this being the new magic model for magic/double magic. At the very least I would prefer an option to keep the older models in use. The model just doesn't seem all that appealing to me compared to the original models

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

This is just a personal opinion from a runner's perspective, but I don't like the idea of this being the new magic model for magic/double magic. At the very least I would prefer an option to keep the older models in use. The model just doesn't seem all that appealing to me compared to the original models

My goal is to allow those models to be used for their original purpose of magic refills, which we could then add as additional junk items. I am happy to take criticism on the models to replace them, but I would choose to remove this PR over making it an option since it would prevent making magic refills an item and defeat the entire point.

@Natheirean
Copy link

This is just a personal opinion from a runner's perspective, but I don't like the idea of this being the new magic model for magic/double magic. At the very least I would prefer an option to keep the older models in use. The model just doesn't seem all that appealing to me compared to the original models

My goal is to allow those models to be used for their original purpose of magic refills, which we could then add as additional junk items. I am happy to take criticism on the models to replace them, but I would choose to remove this PR over making it an option since it would prevent making magic refills an item and defeat the entire point.

Ya, I understand the purpose of it. And maybe its just me being resistant to change, but imagining seeing that as the new magic meter just feels weird to me. And I imagine for people not following the changes for this, that the new model will throw a lot of people off when it gets merged into main dev. Like it would probably warrant an announcement.

@Maplesstar
Copy link
Author

We have the dev changelog which shows changes. I personally believe dev changes don't warrant special announcements. Especially because this is a purely visual change. We've never had announcements for major functional changes, so I see no reason to do this one special. And once a new stable releases, there's the big changelog available for that.

@SlyryD
Copy link

SlyryD commented Sep 5, 2023

Could we instead put a clear tetrahedron around a magic jar (similar to freestanding dins, fw, nl -- https://static.wikia.nocookie.net/zelda_gamepedia_en/images/a/a8/OoT_Din%27s_Fire_Model.png/revision/latest?cb=20130312011354) to indicate the magic meter?

UPDATE: Looks like there's a dlist for that: https://github.com/zeldaret/oot/blob/8913c4fa0d2cec19db6e8b4d8598125977243c5d/assets/xml/objects/object_gi_goddess.xml#L4. I wonder if that can be loaded easily.

@Maplesstar
Copy link
Author

Could we instead put a clear tetrahedron around a magic jar (similar to freestanding dins, fw, nl -- https://static.wikia.nocookie.net/zelda_gamepedia_en/images/a/a8/OoT_Din%27s_Fire_Model.png/revision/latest?cb=20130312011354) to indicate the magic meter?

UPDATE: Looks like there's a dlist for that: https://github.com/zeldaret/oot/blob/8913c4fa0d2cec19db6e8b4d8598125977243c5d/assets/xml/objects/object_gi_goddess.xml#L4. I wonder if that can be loaded easily.

I do think this idea is worth exploring to have a comparison. Perhaps shrink the size of the magic jar slightly to give space for the semi transparent shape. Then if majority opinion is that the bar is not the best option, I will remove this PR. Unfortunately I don't know how to call portions of existing dlists in order to do try it myself.

@SlyryD
Copy link

SlyryD commented Sep 5, 2023

Could we instead put a clear tetrahedron around a magic jar (similar to freestanding dins, fw, nl -- https://static.wikia.nocookie.net/zelda_gamepedia_en/images/a/a8/OoT_Din%27s_Fire_Model.png/revision/latest?cb=20130312011354) to indicate the magic meter?
UPDATE: Looks like there's a dlist for that: https://github.com/zeldaret/oot/blob/8913c4fa0d2cec19db6e8b4d8598125977243c5d/assets/xml/objects/object_gi_goddess.xml#L4. I wonder if that can be loaded easily.

I do think this idea is worth exploring to have a comparison. Perhaps shrink the size of the magic jar slightly to give space for the semi transparent shape. Then if majority opinion is that the bar is not the best option, I will remove this PR. Unfortunately I don't know how to call portions of existing dlists in order to do try it myself.

I'll give it a try -- I'm working on adding magicjars back in (SlyryD@ba9e2a2). I think it's not trivial because we'll need to load the object_gi_goddess file to access the dlist. But that might just be a false understanding.

UPDATE: I managed to load the object file, put the dlist in segment 09, and then reference that dlist. Unfortunately, the dlist assumes the vertices are in segment 06, so it doesn't work properly. I'll see what I can do about that.

UPDATE: Getting somewhere
image

@cjohnson57 cjohnson57 added the Status: Waiting for Release This PR is ready for review or merge, but we're holding off on it until after the next release label Nov 11, 2023
@cjohnson57
Copy link
Collaborator

I'm still worried that sly's model looks a bit too similar to the spells, Maple's can't really be mistaken for anything else lol

@cjohnson57 cjohnson57 removed the Status: Waiting for Release This PR is ready for review or merge, but we're holding off on it until after the next release label Nov 14, 2023
@fenhl fenhl added Status: Needs Review Someone should be looking at it Status: Needs Testing Probably should be tested Status: Under Consideration Developers are considering whether to accept or decline the feature described labels Feb 15, 2024
@fenhl fenhl removed the Status: Needs Testing Probably should be tested label Jun 5, 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 Status: Under Consideration Developers are considering whether to accept or decline the feature described Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants