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.

Sign up for GitHub

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

Jump to bottom

[Enhancement] Song Of Double Time Selector #641

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from
Draft

[Enhancement] Song Of Double Time Selector #641

wants to merge 6 commits into from
+472 −72

Conversation

Dumbledork01
Copy link

@Dumbledork01 Dumbledork01 commented Jun 2, 2024 •
edited by github-actions bot
Loading

This PR will focus on the implementation of an enhancement allowing users to select the time that they want to change to. The idea is to create behavior similar to the 3DS port of Majoras Mask. I'm mainly looking for feedback on my implementation as I update this and for any ideas of how I can make this simpler haha.

Planned Behavior:

  • Song Of Double Time pulls open a menu to select time.
  • The selected time is successfully changed.
  • The opened menu initializes to show the current time.
  • Pressing A will change the time to the selected time, pressing B exits out of the menu and cancels the time change.
  • The user can select a specific day as well.
  • The menu will not allow the user to set a time less than their current time.
  • Selecting a day past their current day allows the user to select any time they want.
  • Setting the Day to Day 2 & the time to 1 am will actually move the time to Day 1 at 1 am. (Since the day technically doesn't start until 6...)

I'd also like the menu to have custom text so I can possibly decouple its behavior from the Bombers' Code behavior, but right now I'm tying it directly to its logic to get it working first.

Build Artifacts

@garrettjoecox garrettjoecox added the enhancement New feature or request label Jun 2, 2024
@Dumbledork01
Copy link
Author

Dumbledork01 commented Jun 5, 2024 •
edited
Loading

@garrettjoecox I think this is at a point where I just need to add custom text. I'm looking through ShipOfHarkinian to see how custom text was handled there, and I found this line https://github.com/HarbourMasters/Shipwright/blob/736dccb00b161a007a387b04267bdc30ef91e0af/soh/src/code/z_message_PAL.c#L1645
I'm guessing I would modify this area of z_message.c to have a similar function?

2ship2harkinian/mm/src/code/z_message.c

Line 3360 in 6fd7fc3

// BENTODO all of these

Any pointers or tips before I try to implement that, or is the message system similar enough between MM & OOT that it shouldn't be too crazy?
Alternatively, I could leave this for a later PR & just have logic that inputs custom text for this instance and worry about setting up a good architecture for it later. That would make my life easier for now haha

@garrettjoecox
Copy link
Contributor

Any pointers or tips before I try to implement that, or is the message system similar enough between MM & OOT that it shouldn't be too crazy?

I'll be a little busy today, but I'll get a proof of concept up for you asap that you should be able to work from

@Dumbledork01
Copy link
Author

Any pointers or tips before I try to implement that, or is the message system similar enough between MM & OOT that it shouldn't be too crazy?

I'll be a little busy today, but I'll get a proof of concept up for you asap that you should be able to work from

@garrettjoecox Don't worry about getting to it today, I'm probably not gonna start back up on this until Friday haha. I appreciate the help :)

@garrettjoecox
Copy link
Contributor

@garrettjoecox Don't worry about getting to it today, I'm probably not gonna start back up on this until Friday haha. I appreciate the help :)

Hey @Dumbledork01 , I know it's been a while but I have pushed another update to the custom message system, if you can't get around to it that's fine but I'd love to see if the new changes are flexible enough to handle your needs here. I combined the two PR's into one because they were a bit intermingled #757

@Dumbledork01
Copy link
Author

@garrettjoecox
Sounds good! I can probably pick this back up next week. Thanks for checking in on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement New feature or request waiting on pr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Dumbledork01 @garrettjoecox