Feature request: Add Left / Right hand information for pianos

[captainerd]

I would like to have this feature, Hand information: Right / Left for piano instruments.

Along side with displaying the tracks of the midi in the expected order since the piano is reversed, the last track is bass, the first is melody, so the bass should appear first then the melody.

I am not sure if done it correctly. a developer could review the code and suggest fixes or direct me in the right way to implement this feature i would appreciate it,

And thanks a lot for this AWESOME project !

[PolyMeilex]

In general the idea sounds good.

But I'm not sure if I like the design, eg.

Even tho I think all cards should have the same height, the gap it creates in organ card looks quite weird.

a) Would something like that do the job as well?

b) Or maybe even something like that:

To be honest I have no idea how to do this nicely, but I think option a) should be good enought for now.

[captainerd]

Well.. that's a headache to me with the design my only relevance to Rust is Tetanus and RD40.. , yesterday i tired it as buttons too and i didn't like it. But i will give it a try again on my day-off, maybe some sort of budge with hand icons would be nice.. ?

anyway, as i was crying on discord from yesterday it jumped notes in arpeggios or in series (when the same note comes less than 500ms as defined in source) so i spend my time now on that as it was more important to me

From my tests, i played "Ludovico experience" and "The River Flows In You" with a nice accuracy.
take a look (/scene/playing_scene/midi_player.rs)

Cheers.. NeoThesiars !

[captainerd]

I've played a few songs, done some testing, everything seems fine now, u can dive in the code check things out

Thanks for the help

[PolyMeilex]

After your changes, shoudn't this be a HashMap<NoteId, UserPress>?
In the code, it looks like you are making sure that there are never two UserPresses with same NoteId pressent.
And when removing from the list you always retain by NoteId as well.

/// So this:
if let Some(index) = required_notes.iter().position(|item| item.note_id == note_id) {
    required_notes.remove(index);
}
// Could become this:
required_notes.remove(note_id);

// This
user_pressed_recently.retain(|item| item.note_id != note_id);
// Can become
user_pressed_recently.remove(node_id);

// This
user_pressed_recently.iter().find(|item| item.note_id == note_id);
// Can become this
user_pressed_recently.get(note_id);

[PolyMeilex]

Suggested change

	pub struct NoteStats {
	song_name: String,
	notes_missed: usize,
	notes_hit: usize,
	wrong_notes: i32,
	note_durations: Vec<NoteDurations>,
	}
	#[derive(Debug)]
	pub struct NoteDurations {
	user_note_dur: usize,
	file_note_dur: usize,
	}
	impl Default for NoteStats {
	fn default() -> Self {
	NoteStats {
	song_name: String::new(),
	notes_missed: 0,
	notes_hit: 0,
	wrong_notes: 0,
	note_durations: Vec::new(),
	}
	}
	}
	impl Default for NoteDurations {
	fn default() -> Self {
	NoteDurations {
	user_note_dur: 0,
	file_note_dur: 0,
	}
	}
	}
	[derive(Default)]
	pub struct NoteStats {
	song_name: String,
	notes_missed: usize,
	notes_hit: usize,
	wrong_notes: i32,
	note_durations: Vec<NoteDurations>,
	}
	#[derive(Debug, Default)]
	pub struct NoteDurations {
	user_note_dur: usize,
	file_note_dur: usize,
	}

[PolyMeilex]

It seems like you are never using the ring buffer aspect of the VecDeque, let's just use a regular Vec and push() instead of push_back()

[PolyMeilex]

Oh, actually it looks like like this should also be a HashMap<NoteId, UserPress>, right? I belive you are making sure that there is no more than one required note per NoteId in the code.

same way as https://github.com/PolyMeilex/Neothesia/pull/172/files#r1614080982

[captainerd]

I just kept the .push_back() i am not sure a double-ended is needed anymore in the code now, it is using only retain, it doesn't handle indexes and positions anymore.. will test with single vecs and if gameplay turns okay i will push

[PolyMeilex]

Is this a check for notes that have not yet ended?
Sounds quite weird to loop over every note in the song just to find the inproggres notes.

[captainerd]

            `  // make it relaxed, Lower Bound: 83% of the file's note duration, Upper Bound: 112% of the file's note duration.

            let lower_bound = duration.file_note_dur as f64 * 0.83;
            let upper_bound = duration.file_note_dur as f64 * 1.12;`
  I am not sure about this one, it seems way too relaxed, maybe make it a little more difficult (file-duration close) ? it is just that there isn't a single point were for a player mentally the note starts and ends (500ms) but if the player wants to learn to play the exact duration then he/she must make a mental -imagination- red line and always aim at the exact same point to start pressing and leaving the key..

playing some difficult songs suggests it should be moved up a little more, like 97% and 105%, ? thats kinda subjective..

[captainerd]

Hey as i am doing a gameplay on every compile i was thinking if i recall correctly the last synthesia used to freeze my waterfall while pressing the correct notes if i also pressed one wrong, it wouldn't accept the correct ones to let waterfall go either, forcing you to stop playing and press only the correct ones,

Its not a huge deal to me i am not fun of that, i don't know about others, it would be something to consider in the future if there are modes like 'melody, play along, auto' or if there are settings like 'Stop on mistakes'

[PolyMeilex]

This is fishy, how is it posible to have negative number of wrong notes? wrong_notes should be u32 rather than i32

[captainerd]

You don't know it is a correct note until 500ms passed, so I consider them all as wrong and subtract later if found to be correct, which may end up subtracting a zero catch by update().. validate that logic I am on the phone can't check out source rn

[captainerd]

We can use saturating_sub() if that's still the case with a u32, unless i stopped doing that subtracting in the later commits in user fn Because when a note pressed is removed from the pressed recently right there before reaching expiration, I am not sure may fn file still does the sub

[PolyMeilex]

Yeha, I would rather just never let the value be invalid, rather than trying to fix the data afterwards. So saturating sub sounds good.

[PolyMeilex]

both count_before and len() are unsigned values therefore count_deleted also is, they can't contain negative values (program will just crache in debug mode if that happens), so either you have to guard agains this value going negative (as it will overflow the intager), or if it can't happen just drop the if, as it does nothing in it's current form.

EDIT: Yeha, just drop the if, there is no way the number of items will increasse after retain.

[captainerd]

It turns out that logic has been removed 3 days ago in this commit of the after-tests debugging:

2379e39

doing

self.user_stats.wrong_notes -= 1;

That, was a stupid idea to begin with. Everything in update() should be always only wrong notes, good ones shouldn't be left inside the collection to expire.

So the rest was just left-over dead code, i cleaned it.

Test done to validate good notes are not left in update() as wrong notes:

1. Let a note sit down for more than 500ms, press it after 2 3 seconds as a slow hit, its never a "Wrong note" must be a "slow hit"