Without further ado, let me introduce 2 projects:
fTz NSF aka libftznsf - a cross platform library for playing NSF files. This is a direct fork of nsfplay where I'll be removing all platform specific parts and rewriting everything in modern c++. This will be a pure ISO C++ library which you can embed into your application.
fTz NSF Player a player application using libftznsf.
Screenshot of the ncurses version:
Screenshot of the Qt version:
When working on an open-source project, keep in mind that making huge sweeping changes (like you did) are highly shunned. If you find a bug, fix that bug, then commit those fixes (in a single commit if possible -- if not, then rebase and roll those multiple commits up into one). If you find another bug, fix that bug, then commit that. Most authors like if you submit a pull request per bugfix as well. And accurate commit messages are absolutely mandatory ("Made it compile in GNU/Linux" is not an accurate portrayal of what you did in that commit).
What you did in that gigantic commit is... well, let's just say I wouldn't merge those changes through either. This isn't a viewpoint of minority, either.
All that said: your efforts to improve something are actually appreciated (yes really! Do not let my above words make you think otherwise!). It's just that how you went about it in this case isn't feasible. For major functional changes like those described, you really need to talk with the maintainer/author first, rather than "just bang out something that works for you". This is what actual collaboration is on open-source projects. Brad is super easy to talk to and accommodating. That communication could have happened either in a GitHub Issues request ("Hey, I want to do this thing, it would involve me changing X/Y/Z, is that an acceptable approach? If not, is there middle ground or alternatives available?"), here on nesdev (there is a NSFPlay thread but a separate one would be OK), or even Email.
As for pull requests: If you do want changes to propagate to my "master" branch, though, you've gotta do it in incremental (i.e. single purpose and as small as possible) changes that don't break backward compatibility with current compilers, don't make non-functional changes (e.g. changes of "coding style"), and of course don't cause a regression.
FWIW, FaTony did communicate with me through e-mail prior to starting work, but I did explain then that I probably wouldn't want to take those kinds of changes (with a "never say never" clause that I'd review any actual changes presented to me, which I did). I'd still consider changes that met those requirements, but the more non-functional refactoring you do, the less this will be possible. If I can't take your changes without changing compilers and changing coding style, then I'm simply not going to take them.
This isn't just a matter of me not liking your style, it's also a matter of making revisions and progressions understandable to everyone using the NSFPlay codebase, which is more than just me and you-- there are multiple forks currently active, not necessarily on github. (Personally I don't like NSFPlay's coding style, but it was inherited from the original maintainer, and I try to follow what's already there and avoid non-functional changes.)
My own plan for NSFPlay is to finish my emulation accuracy goals for one more release of NSFPlay 2, and then begin a total rewrite/refactor for NSFPlay 3 that will have cross-platform compatibility in mind from the ground up, instead of trying to find an incremental way to get there from the existing code. I'm not going to work on it for a while, though, certainly not until a more important project is finished.
Regardless of whether I take pull requests from it, I love that this fork exists. If you only feel comfortable proceeding with the project in your own style, etc. then that's fine, please continue. I just don't want to give you improper expectations about what can go back into my branch.
I do find this statement a bit arrogant, though. It compiles with 0 errors and 0 warnings on the compiler it was written for.FaTony wrote:The initial attempt to compile nsfplay gave me 120 errors and 600 warnings which really speaks about the quality of the code.
You introduced all of these errors and warnings on your own.
Some of the code is quite rubbish, but this has nothing to do with whether the author predicted that someone would try to compile it with GCC ten years later. Just because code is cross-platform clean doesn't mean it's good code, and vice versa.
Code: Select all
Code: Select all
char* something = "foo";
Code: Select all
#ifndef _FILE_H_ #define _FILE_H_
There are classes with virtual functions but without virtual destructor. They have UB.
This is just from what I remember, I'm pretty sure there are other stuff.
And MSVC is a terrible compiler, it was always terrible and it is terrible. If you are on Windows, try Clang.
Ok, rant over.
I'm pretty sure I've seen talks about clang plugin for Visual Studio. I guess it tries to be compatible with MSVC libs. Just search the web.
Since I don't use proprietary software I would just use MinGW + g++ or clang.
I'm not opposed to every change in your pull request, I'm opposed to some of the changes, and also opposed to taking all of them at once.FaTony wrote:Oh boy, I really don't want to start an argument but...
If you had submitted it pull requests that were just minimal revisions for cross-compiler support, I might have considered taking them. I'm not opposed to putting a const on a char* that could use it (that's a good change). I'm not opposed to revising token pasting to work with other compilers (also good). If you'd offer the changes one at a time, they might actually be usable as a pull request. That's not what you offered.
What you offered was entangled with "I don't like the coding style", "I don't like the compiler you're using", "change everything to suit my preferences so you can collaborate with me". If this is your idea of collaborating, yes I suppose it's true that I don't have an interest in it.
I don't fucking care which one is your favourite. You can add support for a new compiler without breaking compilation on the existing one. CHANGING COMPILERS IS NOT SOMETHING YOU DO ON A FUCKING WHIM. It has far-reaching consequences, and is a lot of work, and to top it off you're only working on a subset of the codebase (in particular the GUI is MFC)-- are you expecting me to do the work to change the rest of it to build with your favourite compiler?FaTony wrote:And MSVC is a terrible compiler, it was always terrible and it is terrible. If you are on Windows, try Clang.
If you just want to break working MSVC code and expect me (and all other current users of the code) to either change compilers or do a bunch of work to "unfix" your changes for MSVC so that I can take your pull request, well... don't expect that.
Anyway, I think everyone is OK that we have 2 projects right now. I may do another pull requests and they will be very small and precise. But I don't think it will be in the near future. Again, I understand that you care about backwards compatibility with MFC etc. I didn't intend to break any public API in my pull request and I think I didn't. Those char[_MAX_PATH] that I've changed to std::string were private afaik.
Anyway, I was eager and still am to get stuff done ASAP, that's why I think a full fork will serve it's purpose. I have big plans for it that I'm pretty sure won't match with the goal of nsfplay.
Yes, I'd still love for some version of NSFPlay to be available on other platforms, however you do it.FaTony wrote:I think everyone is OK that we have 2 projects right now.
Haven't ever used it. Does it have Dendy mode and can it force it?mikejmoffitt wrote:Just curious, addressing the first line of OP's post: mplayer seems to do a reasonable job of playing NSF files. What stands out as a problem with it?
And my next project would me making a MIDI exporter which I think doesn't have a free implementation.