search

ZDoom / ZMusic

GZDoom's music system as a standalone library
https://forum.zdoom.org/index.php
60 stars 32 forks source link

CMakeLists.txt: remove options for system GME. #10

Closed ericonr closed 3 years ago

ericonr commented 3 years ago

Add comment about why internal GME is necessary.

Concerns #9

coelckers commented 3 years ago

I'd prefer to have the code commented out, not removed. Who knows, maybe the problem gets resolved in the future.

ericonr commented 3 years ago

If it is added back, it could be retrieved from history, and I guess it would also require adding a version check, right?

If you'd still like to have it commented out, I can fix it later today.

ericonr commented 3 years ago

I think I fixed it, what do you think?

Wohlstand commented 3 years ago

You can use the mark_as_advanced() function to hide all unneeded options, but keep them being tuned if needed in advanced mode.

Blzut3 commented 3 years ago

This pull request is effectively superseded by #20. I agree with Wholstand that the best option is to just hide and discourage the use of the options. Purely from a personal perspective I do believe there shouldn't be any option to use an external GME until the library gets maintained by someone that cares about ABI, but a third party submitted the patch to make it possible to compile separately. I think it's ultimately fair enough to placate the people that feel they absolutely must use system libraries and give them the option to do so and most likely fail to have good results. In my pull request I modified the description of FORCE_INTERNAL_GME to strongly suggest leaving it at the default and added a comment as to why the default is ON.

As a side note, I'm really surprised one of those "every library should be a system library" people never did the same for DUMB.

coelckers commented 3 years ago

This pull request is effectively superseded by #20. I agree with Wholstand that the best option is to just hide and discourage the use of the options. Purely from a personal perspective I do believe there shouldn't be any option to use an external GME until the library gets maintained by someone that cares about ABI, but a third party submitted the patch to make it possible to compile separately. I think it's ultimately fair enough to placate the people that feel they absolutely must use system libraries and give them the option to do so and most likely fail to have good results. In my pull request I modified the description of FORCE_INTERNAL_GME to strongly suggest leaving it at the default and added a comment as to why the default is ON.

It's not just ABI but that our code was changed to work around a bug that got introduced in GME at some point but nobody interested in getting rid of it again. IMO the option needs to be removed entirely with no ability to turn it on again by any kind of force. The code was just commented out in case the situation changes again.

As a side note, I'm really surprised one of those "every library should be a system library" people never did the same for DUMB.

I don't think this one could even work. Our code is derived from foo_dumb so I doubt it is API compatible with the original library.

Wohlstand commented 3 years ago

@coelckers, can you explain to me which bugs were are? About the upstream: some months ago (don't remember when) was work to replace integer types, however, in several cases mistakes was made, one of them caused the bug at the HES playing that caused unexpected randomized tempo changes: https://www.youtube.com/watch?v=eJI5GjzCE7U I fixed that damn, and I had to post the pull-request with that. I also had to polish some other places on it. Try out the most recent from Michael's repository https://bitbucket.org/mpyne/game-music-emu/, will it work correctly, or tell me which formats are broken? I can try to fix some, or if you have some fixes done, let me bring them up into the mainstream.

Wohlstand commented 3 years ago

Also, I see the fix I made in 2019 on KSS playback that was silent because of the same mistake.

coelckers commented 3 years ago

I think it was the KSS bug that forced us to keep our own fork but I am not 100% certain.

Wohlstand commented 3 years ago

Therefore it needs to get some example files to verify the work of them on things and if still broken, fix the bug. At me I have two KSS files: one works fine, second is silent (maybe because it does use an OPLL FM synth chip that isn't supported by GME)