Refactor mainmenu_refresh_music logic

[adamgauthier]

This is just a simple simplification of the mainmenu_refresh_music function in mainmenu.cpp, the decompiled code made it way too hard to understand what it did, I imagine the original code was much closer to this.

[Saibamen]

Can you close and reopen this issue to trigger build with Travis?

[mewmew]

Can you close and reopen this issue to trigger build with Travis?

Done.

[adamgauthier]

Hey, could I get some feedback on this? It's been open for quite a while. 😅
Anything I should change or that doesn't align with the project's goals?

[mewmew]

Hey, could I get some feedback on this? It's been open for quite a while. sweat_smile
Anything I should change or that doesn't align with the project's goals?

Hi Adam!

Thanks for the PR. Normally, your proposed change would be an improvement and thus accepted :) However, the goal of Devilution is to get as close to the original as possible (see #11). This includes suboptimal code, such as using a loop when a simple if-else statement would have been sufficient.

If you would like to clean up this part of the code, then try to make use of the music enum from the PSX debug info to replace magic numbers:

enum {
	TMUSIC_TOWN  = 0,
	TMUSIC_L1    = 1,
	TMUSIC_L2    = 2,
	TMUSIC_L3    = 3,
	TMUSIC_L4    = 4,
	TMUSIC_INTRO = 5,
	NUM_MUSIC    = 6,
};

If you'd be willing to dive even deeper, then install the original compiler (see #11 and #111 for relevant discussions, and the README for usage info) used and try to verify the assembly output.

Hope that helps a bit. You'd be most welcome to contribute to the project. And, as you've gotten a bit more insight into the project goals, you can see that this project is unlike most, where clean/good/refactored code is not better than the alternatives, if the alternative is closer to the original version. This is a preservation project, on which other projects can be built, such as clean-up and refactoring projects: e.g. https://github.com/utilForever/Diablopp

Cheers,
/u

[ghost]

Hey, could I get some feedback on this? It's been open for quite a while

Yeah I'm not really working on the project anymore to be honest. I wanted everyone to focus on fixing the bugs first but at this point it's pretty much complete aside from a few quirks. Basically all that's left is the dirty work, and that will take anything from months to several years to recreate the original byte-byte binary.

Anything I should change or that doesn't align with the project's goals?

Hmm, the logic they chose for this function is indeed very strange and inefficient. The only thing is to make sure that when compiled using optimized VC5 the logic still translates the same. But again, this project is basically dead so I'm not going to test it unless it's a major bug fix.

[ghost]

@galaxyhaxz as I said , let's not go for a byte for byte reverse this could be infinite for a 1 byte differnce.

Let's fork this project (keep it pure ) and cross platform the fork. This will be most beneficial for us.

I opened an issue for this. If you want , I will manage the fork.

[ghost]

The main branch will try to remain identical, but you can send this to the other repo if you want.

[mewmew]

@louistio Feel free to submit the PR to https://github.com/diasurgical/devilution-plus-plus