AntonioND / gbt-player

A music player library for the PSG audio channels of the GB, GBC and GBA.
MIT License
281 stars 20 forks source link

Fix Dnn check from bank1.s to player.s, returns to bank 1 (LegacyGBT) #11

Closed RichardULZ closed 3 years ago

RichardULZ commented 4 years ago

Issue from pull #5 and tweak from pull #8 Songs not in bank 1 would crash on this check as it would remain in the song bank

also reverts :: change as gbdk 2020 was confused, still don't fully understand the : vs :: identifiers.

If i use : now for a called function in another file, under gbdk2020 it spits ?ASlink-Warning-Undefined Global gbt_get_pattern_ptr_banked referenced by module So the new function gbt_get_pattern_ptr_banked:: has :: Technically the old one could remain as : since not called outside the file, but i've reverted it anyway.

This only fixes GBDK Legacy, the issue is likely present in RGBDS if a songs is in bank 2+

AntonioND commented 4 years ago

As I've mentioned in the PR, RGBDS is also affected. Could you also fix that? The fix itself looks fine.

RichardULZ commented 4 years ago

Can't test RGBDS, but i think that should work? :)

AntonioND commented 4 years ago

Sorry, right now I'm on holiday and I can't test anything. I'll be back home this weekend and I'll check.

RichardULZ commented 4 years ago

After rigorous testing, another bug seems to persist due to the Dnn fix, something odd is happening with the first new line being played. My particular case, a note is playing on ch3 near the end of pattern 1, we d00 to the next pattern, which has ch3 c00 as first command, instead of muting a high pitch tone is produced... Removing the call to fix dnn the issue is gone. Will investigate further. (I've fixed many other issues elsewhere, but my private branch for gbstudio is diverging quite a bit from the base with the new effects, as testing in small chunks for release is a lot of effort)

RichardULZ commented 4 years ago

OHHHHH gbt_get_pattern_ptr WAS TRASHING de! WE NEED THAT!!! Will see what else i can clean, as gbt_get_pattern_ptr_banked needs a modified copy of the gbt_get_pattern_ptr routine that uses bc instead.

AntonioND commented 4 years ago

Hmmm... Yeah, this is why I don't like to touch too much this code, there are too many things that aren't documented and that are needed for it to work... :S

AntonioND commented 3 years ago

So I'm wondering if I should revert the original PR that introduced the bug. I'm not sure what bug is worse...

RichardULZ commented 3 years ago

Ahh, sorry about dropping all this, i focused on getting my custom gbt player fork finished up for gbstudio, which is now live https://github.com/chrismaltby/gb-studio/pull/505

This has a working fix for the dnn bugs, but also a lot of other features and changes... If you're interested in digging through it, you should be able to isolate the fix for this, at least in gbdk, not sure when I'll get around to migrating stuff here.

AntonioND commented 3 years ago

Ok, I'll try to find some time in the next few days to do this.

bbbbbr commented 3 years ago

We just had a user that got the `?ASlink-Warning-Undefined Global 'gbt_get_pattern_ptr' referenced by module '' warning/error.

The solution for the time being was to just switch back to the :: colons so gbt_get_pattern_ptr could be global since it's also called from gbt_player_bank1.

So I guess, what's the status on all this?

Maybe a simplified PR with minimal changes so it builds and links properly again?

AntonioND commented 3 years ago

Most people are now using this library through GB studio, which has a fork that has evolved its own way. I'd like to at least make the code in this repository build, of course. Either a quick fix for this, or a revert until it works again.

I'm working a lot on a game for the GBA jam 2021 right now. I can take a look at this afterwards, which should be in a couple of weeks.

AntonioND commented 3 years ago

I've merged this. I've tested it a bit and I don't see any obvious problems, but it has been ages since I wrote this code, so I'm not 100% sure that everything is right.

AntonioND commented 3 years ago

I've also applied this patch: https://github.com/chrismaltby/gb-studio/pull/505/commits/47f46af7b7dd173409a8665ea4b2a0ec9aeb6e5c