fTz NSF Player - a fork of NSFPlay

Discuss NSF files, FamiTracker, MML tools, or anything else related to NES music.

Moderator: Moderators

FaTony
Posts: 34
Joined: Fri Mar 22, 2013 7:41 am

fTz NSF Player - a fork of NSFPlay

Post by FaTony »

Hi there. I really want to listen again to the music of my childhood but there are no good NSF players for my OS - GNU/Linux. So I want to fix this. In 2013 I was messing with NotSoFatso but cancelled the project due to lack of time. Now I'm making a 2nd attempt, this time with NSFPlay. I'm well aware that Brad wants to do a GNU/Linux version in the future but I want it now and have too much free time on my hands.

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:
Image
Screenshot of the Qt version:
Image
Image
Last edited by FaTony on Wed Sep 28, 2016 2:11 pm, edited 4 times in total.
User avatar
koitsu
Posts: 4201
Joined: Sun Sep 19, 2004 9:28 pm
Location: A world gone mad

Re: fTz NSF Player - a fork of NSFPlay

Post by koitsu »

Not to cause drama (this is not my intention, but rather transparency): this isn't a case of "Brad not wanting to collaborate", it's a case of you not actually understanding what it is you're doing (and even admitting it at one point).

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.
User avatar
rainwarrior
Posts: 8731
Joined: Sun Jan 22, 2012 12:03 pm
Location: Canada
Contact:

Re: fTz NSF Player - a fork of NSFPlay

Post by rainwarrior »

I'm happy if someone wants to make a fork that runs on Linux, that in itself is great. I'm actually quite pleased that FaTony is doing this.


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.
User avatar
rainwarrior
Posts: 8731
Joined: Sun Jan 22, 2012 12:03 pm
Location: Canada
Contact:

Re: fTz NSF Player - a fork of NSFPlay

Post by rainwarrior »

FaTony wrote:The initial attempt to compile nsfplay gave me 120 errors and 600 warnings which really speaks about the quality of the code.
I do find this statement a bit arrogant, though. It compiles with 0 errors and 0 warnings on the compiler it was written for.

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. :P
FaTony
Posts: 34
Joined: Fri Mar 22, 2013 7:41 am

Re: fTz NSF Player - a fork of NSFPlay

Post by FaTony »

Oh boy, I really don't want to start an argument but...

Code: Select all

i##(
Parentheses can't be a part of the token.

Code: Select all

char* something = "foo";
"foo" has the type of const char[4]; Assigning it to char* violates constness. It still compiles on my end but with the warning. I guess I'll add -pedantic or -ansi to make it a compiler error.

Code: Select all

#ifndef _FILE_H_
#define _FILE_H_
The ISO C++ reserves names starting with underscore and a capital letter to implementation. All NSFPlay include guards violate this.
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.
tepples
Posts: 22705
Joined: Sun Sep 19, 2004 11:12 pm
Location: NE Indiana, USA (NTSC)
Contact:

Re: fTz NSF Player - a fork of NSFPlay

Post by tepples »

Which C library does Clang for Windows use? In theory, MSVCRT.dll is supposed to be for internal use by Windows.
FaTony
Posts: 34
Joined: Fri Mar 22, 2013 7:41 am

Re: fTz NSF Player - a fork of NSFPlay

Post by FaTony »

It's been a while since I had a PC with Windows on it. Here's what docs say: http://clang.llvm.org/docs/MSVCCompatibility.html

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.
User avatar
rainwarrior
Posts: 8731
Joined: Sun Jan 22, 2012 12:03 pm
Location: Canada
Contact:

Re: fTz NSF Player - a fork of NSFPlay

Post by rainwarrior »

FaTony wrote:Oh boy, I really don't want to start an argument but...
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.

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.
FaTony wrote:And MSVC is a terrible compiler, it was always terrible and it is terrible. If you are on Windows, try Clang.
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?

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.
tepples
Posts: 22705
Joined: Sun Sep 19, 2004 11:12 pm
Location: NE Indiana, USA (NTSC)
Contact:

Re: fTz NSF Player - a fork of NSFPlay

Post by tepples »

MinGW misuses MSVCRT.dll in the way described in the article I linked. The sentiment from the comments to the article was that MinGW developers ought to have developed their own C library instead of depending on an internal component of Windows that is not intended for use by third-party programs (namely MSVCRT.dll).
FaTony
Posts: 34
Joined: Fri Mar 22, 2013 7:41 am

Re: fTz NSF Player - a fork of NSFPlay

Post by FaTony »

I'm sorry that I was too bold. I usually am. I was declared mentally disabled and this prohibits anyone from legally hiring me. Well, at least now I have tons of free time (and no money).

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.
User avatar
rainwarrior
Posts: 8731
Joined: Sun Jan 22, 2012 12:03 pm
Location: Canada
Contact:

Re: fTz NSF Player - a fork of NSFPlay

Post by rainwarrior »

FaTony wrote:I think everyone is OK that we have 2 projects right now.
Yes, I'd still love for some version of NSFPlay to be available on other platforms, however you do it.
User avatar
mikejmoffitt
Posts: 1353
Joined: Sun May 27, 2012 8:43 pm

Re: fTz NSF Player - a fork of NSFPlay

Post by mikejmoffitt »

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?
FaTony
Posts: 34
Joined: Fri Mar 22, 2013 7:41 am

Re: fTz NSF Player - a fork of NSFPlay

Post by FaTony »

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?
Haven't ever used it. Does it have Dendy mode and can it force it?

And my next project would me making a MIDI exporter which I think doesn't have a free implementation.
FaTony
Posts: 34
Joined: Fri Mar 22, 2013 7:41 am

Re: fTz NSF Player - a fork of NSFPlay

Post by FaTony »

Ok, I got basic ALSA playback done. Also attached screenshot of the console version to the 1st post. Next I will be getting Qt GUI that will be in sync with console version.
FaTony
Posts: 34
Joined: Fri Mar 22, 2013 7:41 am

Re: fTz NSF Player - a fork of NSFPlay

Post by FaTony »

Qt version is now on par with console one. Screenshot in the 1st post.
Post Reply