Skip to content

Conversation

Kade-github
Copy link

Addresses #208

Allows for the switching of sides in a double chart (8 columns)

@Kade-github
Copy link
Author

Kade-github commented Sep 6, 2025

I think there's probably a myriad of things wrong with this so I would love feedback, mostly on how default shortcuts are made and what not, if the placement is correct, and if this code quality is acceptable.

Thank you in advance!

EDIT: Also, theres a part in this where it deletes the notes of the selection, then it adds in the new notes. This creates two actions which means two undos! I don't know how to merge these two actions together because the clear flag on modify seems to leave the last note alone for some reason!

@StarbotArc
Copy link
Collaborator

Yeah this is hardcoded to be dance-doubles and nothing else. Style does have an id and number of columns variable that you can use, so you can fix that part easily.

Also, you're pointing at the release instead of the beta branch, which is the development branch here.

Other than that, it looks fine from a first glance.

@Kade-github Kade-github changed the base branch from release to beta September 6, 2025 16:47
@Kade-github Kade-github force-pushed the kade-github/change-side branch 3 times, most recently from 279f3b2 to bde6c12 Compare September 6, 2025 17:03
@Kade-github
Copy link
Author

Yeah this is hardcoded to be dance-doubles and nothing else. Style does have an id and number of columns variable that you can use, so you can fix that part easily.

Also, you're pointing at the release instead of the beta branch, which is the development branch here.

Other than that, it looks fine from a first glance.

Hi! Thanks for the feedback, I went ahead and changed the target branch to the beta branch (my bad, I didn't know that lol). Commits should all be cleaned up for it now.

My implementation also checks for the number of columns, not the style name (though at first it did, not sure if I pushed that here though.)

@Kade-github Kade-github force-pushed the kade-github/change-side branch from 7af7a74 to 79875e0 Compare September 6, 2025 18:58
@Kade-github
Copy link
Author

Had to fix some formatting stuff inside of Shortcuts, just ran clang-format on it. Also fixed the double undo bug I was talking about, but now it's triggering the warning message for when a note is added on the same row. Hmm

@StarbotArc
Copy link
Collaborator

My implementation also checks for the number of columns, not the style name (though at first it did, not sure if I pushed that here though.)

My problem is that you're not really checking for any other styles identified as double, and instead is just focused on dance-double exclusively. Style has an identifier variable unrelated to name that can be used here.

In other words, style.id could potentially be dance-double which has 8 columns, or pump-double which has 10 columns, which you're not checking and just assuming that everyone uses dance-double to be blunt. Not to mention we could have random styles with their own double variants.

Hopefully you understand what I am trying to say, I may have not been too clear on the first comment, that's my fault.

@Kade-github
Copy link
Author

Kade-github commented Sep 7, 2025

My implementation also checks for the number of columns, not the style name (though at first it did, not sure if I pushed that here though.)

My problem is that you're not really checking for any other styles identified as double, and instead is just focused on dance-double exclusively. Style has an identifier variable unrelated to name that can be used here.

In other words, style.id could potentially be dance-double which has 8 columns, or pump-double which has 10 columns, which you're not checking and just assuming that everyone uses dance-double to be blunt. Not to mention we could have random styles with their own double variants.

Hopefully you understand what I am trying to say, I may have not been too clear on the first comment, that's my fault.

Yeah I think I misunderstood what you were trying to say, but I think I get it now:

HudNote("Switch side is only available in a double style.");

Is explicitly stating that it needs to be a double style, in which it does not check if it is a double style, but just if it has 8 columns.

Maybe it'd be better if this somehow detected what a 'side' is programmatically? Ie, maybe split down the middle; and have it check like that? Like if you have 6 columns, 3 per side. And anything below 4 wouldn't count as a style with a "side."

Though numbers like 5, I just don't know. So maybe it should be limited to just 8 columns, or maybe even numbers like 8.

@StarbotArc
Copy link
Collaborator

I'll just give you an example of what ideally it should do:

if (!style.id.endsWith("double")) { /* do thing here */ }

int halfpoint = style.numCols / 2;

for (Note note : notes) {
 if (note.lane < halfpoint) {
  note.lane += halfpoint;
 } else {
  note.lane -= halfpoint
 }
}

Pretty much, you're just using the variables that style already has to determine how it should swap stuff. You might also get the idea to check the amount of columns can cleanly divide by two, but this is what I am trying to get across.

@Kade-github
Copy link
Author

Yeah that's a good idea, will do that.

@uvcat7 uvcat7 self-requested a review October 3, 2025 05:37
return;
}

if (!style.id.endsWith("double")) {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (!style.id.endsWith("double")) {
if (!style->id.ends_with("double")) {

The string functions have changed to use std::string instead of Vortex::String

}

NoteEdit edit;
gSelection->getSelectedNotes(edit.add);
Copy link
Owner

Choose a reason for hiding this comment

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

This section doesn't work because the note array of the selection shares the same pointer.
You'll need to build one of the lists by hand.


NoteEdit edit;
gSelection->getSelectedNotes(edit.add);
edit.rem = edit.add;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
edit.rem = edit.add;

n.col -= halfpoint;
}
}

Copy link
Owner

Choose a reason for hiding this comment

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

The notes in edit.add need to be in the correct order to avoid an error message.
Just copy the code in mirrorNotes() that sorts them to here.

Copy link
Owner

@uvcat7 uvcat7 left a comment

Choose a reason for hiding this comment

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

This code also has some clang format violations.
If you're using Visual Studio, you can reformat the offending files with Ctrl+K, Ctrl+D.

Otherwise, looks good! Sorry for merging into your branch, I had screwed up the formatting in Shortcuts.cpp and didn't want to make you handle the merge conflict.

You should be able to run the CI now. Let me know if it's not working, because I can run it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants