Skip to content

[BUGFIX] Make strumline backgrounds no longer layer over UI elements #5152

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

Open
wants to merge 44 commits into
base: develop
Choose a base branch
from

Conversation

stringfromjava
Copy link

this pull request fixes this issue: #4541

fixes mildly annoying layering issues with the strumline backgrounds being layered over the ui

Before...

image

After!

image

@github-actions github-actions bot added status: pending triage Awaiting review. pr: haxe PR modifies game code. size: large A large pull request with more than 100 changes. labels May 24, 2025
@AbnormalPoof AbnormalPoof added type: minor bug Involves a minor bug or issue. status: needs revision Cannot be approved because it is awaiting some work by the contributor. and removed status: pending triage Awaiting review. labels May 24, 2025
Copy link
Member

@AbnormalPoof AbnormalPoof left a comment

Choose a reason for hiding this comment

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

  1. This makes the health bar go above the arrows, which inadvertently makes them harder to see. The solution is to make the health bar ABOVE the strumline background, but BELOW the arrows. This makes the logic for the strumline backgrounds way more annoying though.
  2. Your code has unnecessary changes that aren't related to the PR. Please revert them.
  3. Please change the base branch to develop.

Comment on lines 481 to 489
/**
* The base character for the opponent.
*/
var dad:BaseCharacter;

/**
* The base character for the player.
*/
var boyfriend:BaseCharacter;
Copy link
Member

Choose a reason for hiding this comment

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

These are not necessary.

Comment on lines 569 to 573
/**
* The current data contained for BOTH characters. This is data from the current
* chart being played by the user.
*/
var currentCharacterData:SongCharacterData;
Copy link
Member

Choose a reason for hiding this comment

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

This is not necessary.

@stringfromjava
Copy link
Author

stringfromjava commented May 24, 2025

after some thinking, why cant we just make the backgrounds their own separate sprites in the playstate? ik that might be frustrating a little bit but thats the only reasonable solution i can really think of (bc as far im thinking abt it, its not really possible to achieve what is wanted unless that happens)

@Lasercar
Copy link
Contributor

Lasercar commented May 24, 2025

The background is in the strumline so that it follows the strumline when it's moved, and hidden when the strum it's attached to is hidden.

@EliteMasterEric
Copy link
Member

Yep.

The conundrum here is that since the background is part of the strumline to keep the logic for it simple, but that also means that it's grouped together with the notes.

I'm personally wondering if having the health bar go above the arrows is really that big of a deal, though.

@anysad
Copy link
Contributor

anysad commented May 24, 2025

Isn't the whole purpose of the strumline background to lower the amount of distractions so you can focus on the notes? (so that combo, rating and other stuff wouldn't get in the way)

@stringfromjava
Copy link
Author

stringfromjava commented May 25, 2025

Yep.

The conundrum here is that since the background is part of the strumline to keep the logic for it simple, but that also means that it's grouped together with the notes.

I'm personally wondering if having the health bar go above the arrows is really that big of a deal, though.

i was wondering that too, since it really doesnt have that much of an impact imo

it just looks cleaner to me, plus i dont think anyone would care or even notice whether the arrows were relayered or not :p

@JackXson-Real
Copy link
Contributor

Yep.

The conundrum here is that since the background is part of the strumline to keep the logic for it simple, but that also means that it's grouped together with the notes.

I'm personally wondering if having the health bar go above the arrows is really that big of a deal, though.

Its really not noticeable. There is no way that it will affect anyone's gameplay imo unless they specifically look at the bottom of the screen for some reason (or top if downscroll), but nobody does that. Plus, the health bar is really small anyways, so it really wouldn't get in the way.

Copy link
Member

@AbnormalPoof AbnormalPoof left a comment

Choose a reason for hiding this comment

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

Still looking for clarification on these:

#5152 (comment)

#5152 (comment)

@stringfromjava
Copy link
Author

sorry ive been busy and went out of town. im most likely going home today so ill fix it when i get home (sent thru ny phone)

@stringfromjava stringfromjava changed the base branch from main to develop June 1, 2025 19:55
@github-actions github-actions bot added size: medium A medium pull request with 100 or fewer changes. and removed size: large A large pull request with more than 100 changes. labels Jun 1, 2025
@stringfromjava
Copy link
Author

i have made the requested changes

@AbnormalPoof AbnormalPoof added status: pending triage Awaiting review. and removed status: needs revision Cannot be approved because it is awaiting some work by the contributor. labels Jun 1, 2025
[DOCS] Fix a link in a Changelog entry
@Hundrec Hundrec linked an issue Jul 9, 2025 that may be closed by this pull request
3 tasks
@github-actions github-actions bot added size: huge A huge pull request with more than 500 changes. and removed size: medium A medium pull request with 100 or fewer changes. labels Jul 31, 2025
@github-actions github-actions bot added pr: documentation PR modifies documentation or README files. pr: github PR modifies GitHub metadata files. labels Aug 1, 2025
@stringfromjava
Copy link
Author

I have made fixed the merge issues.

@Hundrec Hundrec added status: pending triage Awaiting review. and removed status: needs revision Cannot be approved because it is awaiting some work by the contributor. pr: documentation PR modifies documentation or README files. pr: github PR modifies GitHub metadata files. labels Aug 1, 2025
@JackXson-Real
Copy link
Contributor

Thanks for fixing the merge conflicts, though I'm surprised it only took you 45 commits to fix them, I'm impressed!

@Hundrec Hundrec changed the title [BUGFIX]: strumline backgrounds no longer layer over the entire ui [BUGFIX] Make strumline backgrounds no longer layer over UI elements Aug 1, 2025
@stringfromjava
Copy link
Author

stringfromjava commented Aug 1, 2025

Thanks for fixing the merge conflicts, though I'm surprised it only took you 45 commits to fix them, I'm impressed!

Thank you! I tried my best, I'm new to contributing to public projects, so I'm trying to learn how the system usually works when I try to get a job.

@AbnormalPoof AbnormalPoof added status: needs revision Cannot be approved because it is awaiting some work by the contributor. and removed status: pending triage Awaiting review. labels Aug 1, 2025
@github-actions github-actions bot added size: large A large pull request with more than 100 changes. pr: documentation PR modifies documentation or README files. pr: github PR modifies GitHub metadata files. and removed size: large A large pull request with more than 100 changes. size: huge A huge pull request with more than 500 changes. labels Aug 2, 2025
@Hundrec Hundrec added size: large A large pull request with more than 100 changes. and removed pr: documentation PR modifies documentation or README files. pr: github PR modifies GitHub metadata files. labels Aug 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: haxe PR modifies game code. size: large A large pull request with more than 100 changes. status: needs revision Cannot be approved because it is awaiting some work by the contributor. type: minor bug Involves a minor bug or issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug Report: UI Elements are layered below Strumline Background