Bug: `inc a` distorts channel 4 percussion in `ReadNoiseSample`

[TempoQuill]

This issue ranges from SW97 all the way to Crystal. In ReadNoiseSample, the first byte is split into a purely cosmetic hi nybble to make importing RBY noise data a matter of copy-paste, and a lo nybble for the length.

All the RBY percussion is already there, and drums that are two or more notes long indicate the same note length structure as in RBY. However, because of the way ReadNoiseSample stores this first byte, this length is one frame longer than intended, meaning minimum note length is two instead of one. This makes the old drums with two or more notes sound slower than they should be.

Would this be counted as a bug?

    inc de

    cp sound_ret_cmd
    jr z, .quit

    and $f
    inc a
    ld [wNoiseSampleDelay], a

[aaaaaa123456789]

No, that's a conversion error. A length of zero is nonsense; the duration being forced to a range of 1-16 is reasonable and matches the behavior of the other channels.

[Rangi42]

If the GSC music sounds the way it's supposed to, then this isn't a bug.

[TempoQuill]

No, that's a conversion error. A length of zero is nonsense;

The intended length is 1-16, but the inc a makes it so $x0 means two frames rather than one.

    and a
    jr z, ReadNoiseSample
    dec a
    ld [wNoiseSampleDelay], a

When we arrive back at the very end of HandleNoise, wNoiseSampleDelay instructs the number of updates before we reach ReadNoiseSample again. If wNoiseSampleDelay says zero, that means ReadNoiseSample is accessible at that moment. If wNoiseSampleDelay is incremented beforehand, that means drum notes can never be shorter than two frames. Instead of 1-16, it's 2-17.

[vulcandth]

Would changing this alter the way GSC music sounds? If so, then this becomes a subjective change.

I think in discord you mentioned that it is handled differently in RBY? What is the benefit to changing this?

Also, if this is something we should document... it would probably be more of a design flaw imo.

[TempoQuill]

In RBY, drums were pretty much one of the only things that REALLY worked properly. It had one byte for a length of 1-16 frames, designated by 2x. This was very consistent with the sound effect data, which was ALSO designated with 2x on the first byte.

In GSC, Gen 1's percussion was imported as is, because of the cosmetic hi nybble. By removing the inc instruction before storing the length in RAM, the Gen 1 drums would sound as they did in RBY. In addition, the newer percussion becomes cleaner and clearer.

[Idain]

The "cleaner and clearer" is a subjective matter. Also, not being able to import Gen 1 songs into Gen 2 because of differences in drums sounds more like a design flaw than a bug, since they never planned for this to happen.

It could be documented as a design flaw, but definitely not a bug.

[Idain]

It'd be cool if you posted the songs somewhere without the inc a instruction to see how they'd sound like so people could make a better judgement whether to "fix" it or not.

YouTube could be perfect for it, or even Soundcloud.

[vulcandth]

YouTube could be perfect for it, or even Soundcloud.

Careful with Youtube. Good chance the original would get flagged for a copyright claim. Even the edited one, could get flagged.

Edit: Also isn't there other blockers that prevents Gen 1 songs from being imported to Gen 2? I was under the impression the formats are quite different.

[TempoQuill]

I was under the impression the formats are quite different.

Different enough you couldn't copy-paste on HxD, but you could tie the two formats together somewhat with identical macros that work differently depending on the format.

EDIT: The only commands from Gen 1 that are not in Gen 2 are toggle_perfect_pitch/pitch_inc_switch, global stereo and execute_music/set_music.

[Idain]

All of this still sounds like a design flaw rather than a bug. After all, they never intended to use Gen 1 songs in GSC.

[Idain]

Careful with Youtube. Good chance the original would get flagged for a copyright claim. Even the edited one, could get flagged.

@vulcandth, I was talking about only uploading the modified versions. Also, it's hard for Youtube to catch new accounts with almost no subs, so I wouldn't worry about it. There are 12+ years videos with Pokémon music and they haven't been flagged yet.

[vulcandth]

@vulcandth, I was talking about only uploading the modified versions. Also, it's hard for Youtube to catch new accounts with almost no subs, so I wouldn't worry about it. There are 12+ years videos with Pokémon music and they haven't been flagged yet.

I don't know what their youtube status is. They could be Mr. Beast for all I know. Also, yes the risk is low.. but it is not zero. It it up to them to determine their own risk tolerance.

[Idain]

That's a good point too. Anyways, I'm in favor of documenting this as a design flaw rather than a bug.

[TempoQuill]

https://soundcloud.com/aizakku-horooee/drum-test-ss-aqua Got the test for anyone else on here to decide a favor. Anyway, if it's being documented at all, might as well be a design flaw.

[vulcandth]

As I stated before I believe this change to be subjective. With that being the case, we need to determine if we want subjective changes in design flaws doc. Personally I would say no, and leave all subjective changes for wiki tutorials. What do you think @Rangi42 ?

[Idain]

You have a good point there. A wiki tutorial for it with a disclaimer stating that it may affect Gen 2 songs as well.

[vulcandth]

@TempoQuill Based on the discussion that occurred in discord, I believe the determination from Rangi was that this may indeed be something that could be considered a bug/design flaw, and that the fix should be documented in a wiki page first so she may look it over further. It should be pretty easy to port from the wiki to the repo docs/ later. Would you have time to start the wiki page?

[rawr51919]

I'll give it a shot, sounds like something that could be made a stub at first, and then as info about this is figured out it could be expanded and eventually evolve into a design_flaws.md contribution.

Edit: Wiki page made at https://github.com/pret/pokecrystal/wiki/Fix-ReadNoiseSample-instrumentation-distorting-percussion, tweak as you see fit

Edit Edit: PR has been made for this, when Rangi has time she can look at it (#944)

[rawr51919]

@Rangi42 would the wiki page be sufficient for this? Considering the PR's already been thanos'd I might as well ask If it is, we can close this one

[Rangi42]

The wiki page was you, haven't heard from @TempoQuill re: what else should be added, cleaned up, whether it's accurate, etc.

[rawr51919]

@TempoQuill Is this wiki page suitable for this now?

The wiki page has been added to the tutorials to make it easy to find and make this tweak