search

logicomacorp / WaveSabre

Official WaveSabre repository
MIT License
246 stars 34 forks source link

Factor out sample loading logic from Thunder and Specimen into a separate file #52

Closed PoroCYon closed 3 years ago

PoroCYon commented 4 years ago

Makes porting stuff etc. easier later on.

yupferris commented 3 years ago

Quick note on why I haven't accepted/merged this yet: it looks good, but I need to make sure it doesn't increase compressed size (by an unreasonable amount). A (competent) user will never use Specimen and Thunder in the same song (they shouldn't use Thunder at all!), so this decoupling will likely only negatively affect code size from a user's perspective.

PoroCYon commented 3 years ago

I suppose I could change things a bit so that the Specimen.LoadSample/Thunder.LoadSample functions aren't inline, and then make SampleLoader::LoadSampleGSM inline so that with a competent compiler*, the binary output will be more or less the same as before these changes.

Does that sound good?

*: I have some experience with nudging GCC and clang towards doing this (eg. in my 4k introsystem, which is actually quite a number of tiny and not-so-tiny functions, but it all gets inlined into one or two big functions in the output), but not MSVC.

kusma commented 3 years ago

__forceinline might be what you want for that...

PoroCYon commented 3 years ago

Here's a commit that does that. If it's desireable, I'll squash it into the other one.

yupferris commented 3 years ago

I finally got around to testing this size-wize (after rebase); using Reverence.als from the converter tests and squishy 1.3.0, current master is 29056 bytes, whereas this branch is 29068 bytes. As expected, slightly worse, but only by 12 bytes, which is quite OK imo! I have some other nits that I think I'll test out next, and we'll see where we end up then, but so far, looking good.

yupferris commented 3 years ago

So for an additional 12 bytes (lol), I was able to simplify things further by refactoring SampleLoader into a GsmSample class instead, which unifies a lot of the memory management code (now the devices just manage a single sample instance, and the various other buffers etc are handled internally to that). You can see my changes on this branch. Thoughts?

yupferris commented 3 years ago

Superceded by #72.