search

bbbradsmith / nsfplay

Nintendo NES sound file NSF music player
https://bbbradsmith.github.io/nsfplay/
279 stars 43 forks source link

Create separate "win" static library for Windows/plugin-specific functions #24

Closed jprjr closed 4 years ago

jprjr commented 4 years ago

Hey there! This is definitely a bigger PR from before, but I tried to keep the actual code changes to a minimum. Most changes are to the solution file, re: search paths for includes and required static libraries for the output DLL files.

This creates a new static library, win. In PR #23 I was thinking of making several new libraries, I decided it may be easiest/simplest to just create a single library, since they're functions shared by so many other libraries.

I moved the plugin folder of the xgm library into there, along with the NSF_TAG module, and created a new NSFPlayerConfigIni subclass of NSFPlayerConfig. NSFPlayerConfigIni has the Load/Save methods for using INI files. I made sure to update every build configuration, and tested the build of each module individually to ensure I've got the right build dependencies and so on.

I thought about creating a new namespace for this library, but in the interest of keeping code changes small, I opted not to.

I've also added a Makefile (in the contrib folder) that builds a static and shared library of xgm + vcm (or at least, the parts of vcm needed by xgm). I was able to build said library on my Mac, and use it to load & decode an NSF and spit out a WAV file. I plan to add nsf2wav as a demonstration program at a later date, after cleaning it up some, adding comments, etc.

bbbradsmith commented 4 years ago

This reorganization seems OK to me. Two questions:

  1. I noticed you've added an x64 build target to the VS project. Was that intentional? I know that making sure at least XGM is 64-bit safe is necessary for porting to Linux or Mac, but I don't have any intention of making a 64-bit Windows release of NSFPlay 2. NSFPlay 3 on the other hand will have a 64-bit target from day one, but the process of evaluating and updating the 20-years old code of NSFPlay 2 for 64-bit safety seems like a waste, for the Windows platform at least.

Edit just to clarify what I'm asking: do you need/want the 64-bit target in the MSVC projects to accomplish what you're doing, or can I remove it?

  1. Why is the folder called "contrib"? I'm not opposed to the name (and the old names for the various libraries and projects in here are certainly confusing), but I'm wondering what it means? I would have expected something like "build".
jprjr commented 4 years ago

For question 1 - that wasn't intentional, I think that may have been added automatically, I'm fine with removing it.

For question 2 - maybe a better name may be "unix" ? I often see "contrib" used for things like plug-ins or scripts that aren't "officially" supported by the project. I wanted to try and convey "hey, this is still being worked on/experimental" In hindsight, I think the README.md file makes that pretty clear, so renaming it something like "build" or "unix" or something else entirely seems fine.

On Sat, Feb 08, 2020 at 10:01:48AM -0800, Brad Smith wrote:

This reorganization seems OK to me. Two questions:

  1. I noticed you've added an x64 build target to the VS project. Was that intentional? I know that making sure at least XGM is 64-bit safe is necessary for porting to Linux or Mac, but I don't have any intention of making a 64-bit Windows release of NSFPlay 2. NSFPlay 3 on the other hand will have a 64-bit target from day one, but the process of evaluating and updating the 20-years old code of NSFPlay 2 for 64-bit safety seems like a waste, for the Windows platform at least.

  2. Why is the folder called "contrib"? I'm not opposed to the name (and the old names for the various libraries and projects in here are certainly confusing), but I'm wondering what it means? I would have expected something like "build".

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/bbbradsmith/nsfplay/pull/24#issuecomment-583761308