search

jkotlinski / lsdpack

Standalone LSDj recorder+player
GNU General Public License v2.0
116 stars 18 forks source link

Implement a register dump mode, selected with '-d' #17

Closed TheOtherDays closed 3 years ago

TheOtherDays commented 3 years ago

For this, we need to convey the gameboy cycles to the writer.

At least 2 problems are remaining:

jkotlinski commented 3 years ago

Hi! Thanks for the contributions!

Regarding problems

  1. Song does not start immediately, there is a delay from selecting and loading the song. You want to offset the cycle counter so that 0 is at song start.
  2. Recording happens in 2x speed CGB mode, I suppose that's why there are 2x as many cycles as expected.
cyberic99 commented 3 years ago
  • Song does not start immediately, there is a delay from selecting and loading the song. You want to offset the cycle counter so that 0 is at song start.

yeah I noticed that, I reset the counter here: https://github.com/jkotlinski/lsdpack/pull/17/commits/032dd7f4fa57830de6b03d08c31f5e5c2606a789#diff-40c03e546ed3d507cf96ddff3e50f8e3a26c496a389e7fb1f744f0e96f5ae19fR89

  • Recording happens in 2x speed CGB mode, I suppose that's why there are 2x as many cycles as expected.

right. Do you this an option to select the gb/dmg mode would be useful?

jkotlinski commented 3 years ago

I’m not sure :D What is the use case for these new features?

On Tue, 16 Feb 2021 at 13:37, cyberic99 notifications@github.com wrote:

  • Song does not start immediately, there is a delay from selecting and loading the song. You want to offset the cycle counter so that 0 is at song start.

yeah I noticed that, I reset the counter here: 032dd7f

diff-40c03e546ed3d507cf96ddff3e50f8e3a26c496a389e7fb1f744f0e96f5ae19fR89

https://github.com/jkotlinski/lsdpack/commit/032dd7f4fa57830de6b03d08c31f5e5c2606a789#diff-40c03e546ed3d507cf96ddff3e50f8e3a26c496a389e7fb1f744f0e96f5ae19fR89

  • Recording happens in 2x speed CGB mode, I suppose that's why there are 2x as many cycles as expected.

right. Do you this an option to select the gb/dmg mode would be useful?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jkotlinski/lsdpack/pull/17#issuecomment-779810240, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAY34OZD4AW3UX4S3TD5UYLS7JRKFANCNFSM4XWERVAQ .

cyberic99 commented 3 years ago

I’m not sure :D OK, then. I can still divide the cycles clock afterwards

What is the use case for these new features?

we (the other days band) use this kind of dumps to replay these registers on real hardware. but maybe this is a bit too specific. I think the raw reg dump could still be useful for learning purposes.

So feel free to accept or reject this pull request, I can still apply the patch manually if needed.

Anyway, thanks for lsdpack, it is useful to export lsdj songs to the outside world ;-)

jkotlinski commented 3 years ago

Two Three Four thoughts:

What do you think? If IWriter seems like too much work, I can probably add that post-merge.

cyberic99 commented 3 years ago 
* would it be fine to remove the -r flag, if -d serves your purpose better?

yes. in fact, the -d was what I called 'raw register dump', but after seeing my feature request, you implemented the -r.

so let's forget the optimized dump and the '-r' if you agree.

* how about adding an IWriter interface, and have two implementations of it, DumpWriter and GbWriter? I think it would make things simple and clean.

I can try to do that but I'm rather familiar with plain C rather than with C++. I hope it won't be too ugly ;-)

* changelog should be updated. You could perhaps add new Added section in Unreleased, briefly describing the added flag and what it does.

Of course. But first I wanted to know if you would agree on the feature itself.

* I don't see any benefit in recording in DMG mode. CGB will have better accuracy and sound better thanks to the extra speed. Unless there are particular technical reasons to record in DMG mode, I would avoid it.

For our use, it is useful, because we replay the registers on DMGs which have a better sound. But yeah, it is certainly too specific so we can drop the idea.

What do you think? If IWriter seems like too much work, I can probably add that post-merge.

I think it is better to keep the git history clean, so if you agree, I will edit my pull request, and once you are happy with it, you can merge it.

jkotlinski commented 3 years ago

Hallo :) I would be perfectly fine with the following outcome

cyberic99 commented 3 years ago 
* how about adding an IWriter interface, and have two implementations of it, DumpWriter and GbWriter? I think it would make things simple and clean.

For now, we have 3 functions: record_gb, record_gbs, and record_dump. Do you want to merge both record_gb, record_gbs into the IWriter ?

what do you think if instead of an interface, DumpWriter and GbWriter would inherit from Writer ?

jkotlinski commented 3 years ago

Hallo! I was imagining IWriter as an abstract interface, something like this:

class IWriter {
    public:
        virtual void record_song_start(const char* out_path) = 0;
        virtual void record_song_stop() = 0;
        virtual void record_write(unsigned char addr, unsigned char data, unsigned long cycles) = 0;
        virtual void record_lcd(unsigned long cycles) = 0;
        virtual void write_music_to_disk() = 0;
};

I did not consider moving record_gb, record_gbs, record_dump into IWriter. In their current form, I think it would be difficult to do so cleanly.

jkotlinski commented 3 years ago

@cyberic99 hey! I wanted to fix outstanding stuff, so I merged your commit and made -d work. I hope this does not cause any inconvenience for you.

I have not added single speed option, but future PR's are welcomed. Thank you again for the contribution :-)

cyberic99 commented 3 years ago

hey @jkotlinski ! sorry, I didn't have much time to work on this.

I don't know if I'll use the single speed option, I think I'll just halve the cycles count.

Thank you, and thank you for your work on LSDj !