Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New mapper code #612

Merged
merged 14 commits into from
Sep 20, 2024
Merged

Conversation

negativeExponent
Copy link
Contributor

@negativeExponent negativeExponent commented Sep 19, 2024

lots of changes. see if there is anything worth changing first before we process ig a big merge. Atm, this is the jist of the entire mapper rework, modified to work with this codebase. Regressions might be in place, maybe some incorrect conversions. better alot can test other than just me. The other modifications can be added later bits by bits.

See 1st commit for majority of the change.

BTW, with regards to core options for language translations... is there a way to make it easier to add or remove options without having to do it over and over againt for every language that was added?

@LibretroAdmin @hunterk @hizzlekizzle for review

btw, some unused code left behind, will be cleaned up later when things work or is stable enough.

- Mapper code rewrite, fixes and updates, additions
- Expansion Audio updates
- update VRC7 sound code (emu2413)
- replace Sunsoft 5B sound code with emu2149 for better accuracy (including envelop and noise emulation)
- Update FDS sound expansion (fds audio code from Mednafen with
  modification for fceumm' use)
- Update NSF support
- Add NSFE support
- others (maybe)
Require for some mappers aka Game Doctor, etc
Adding it as-is
@hizzlekizzle
Copy link
Contributor

oh wow, big PR! This will take a hot minute to review intensively, so is there anything we should be on the look out for in a quicker skim?

@negativeExponent
Copy link
Contributor Author

code-wise, not sure. maybe a heads up especially for those relying on savestates since this will be broken in this PR (and the succeding PRs, probably)

i DO need to know if there is a better way to handle core options for translations. its kinda tedious to add/remove/change options since you have to deal with all languages one by one to update.

maybe, a separate macro/inline/define file for each language used, then a common struct which labels will be replaced depending on language. right now, you have to make/update struct on each.

@LibretroAdmin
Copy link
Contributor

BTW, with regards to core options for language translations... is there a way to make it easier to add or remove options without having to do it over and over againt for every language that was added?

Well, translations are done on the CrowdIn platform, whenever ppl submit new translations there, the repo pulls in new code.

Also I gave you write permissions for this repo too. Not sure if the invite expired, it was sent sometime ago.

@LibretroAdmin LibretroAdmin merged commit f1d7589 into libretro:master Sep 20, 2024
1 check passed
@LibretroAdmin
Copy link
Contributor

Looks alright to me and first testing seems OK.

I'll be asking dusanfamicom for some more thorough testing. He can file new issue reports based on his findings.

@hizzlekizzle
Copy link
Contributor

crowdin

Yes, this is what I was thinking, as well, but wasn't sure. In that case, you don't need to worry about anything but the english language and then the others are handled by volunteers via crowdin.

@negativeExponent
Copy link
Contributor Author

did it break crowdin fetch thingie though?

@negativeExponent negativeExponent deleted the new_mapper_code branch September 20, 2024 03:37
@LibretroAdmin
Copy link
Contributor

I don't think it should but we'll find out soon enough if/when ppl complain on Crowdin.

@negativeExponent
Copy link
Contributor Author

Also I gave you write permissions for this repo too. Not sure if the invite expired, it was sent sometime ago.

it said expired.

@LibretroAdmin
Copy link
Contributor

LibretroAdmin commented Sep 20, 2024

Hi,

I got some feedback from dusan.

all mappers do not work, the ones for multicart

I asked several times if he could be more specific but that is all what he came back with. He says these mappers did work before this PR.

Is it perhaps possible you can leave out the refactors, maybe revert the PR and just pull in the other changes/additions instead that won't cause possible regressions to existing mappers? Basically we might have to be more careful with the mapper refactors to avoid breakage of existing mappers.

@negativeExponent
Copy link
Contributor Author

negativeExponent commented Sep 20, 2024

nothing can be fixed IF no details are provided. if it worked before, then its just a minor fix in the refactor, if its even a code error and not human error. since this was practically "refactored/converted" from my own repo which was already tested (at least for the roms file i have at hand).. the modifications are too many to do this all over again one commit at a time...

you can just revert this repo to what it was and make it a static core, make mine a new repo if you want. but i wont be porting the modifications again one by one. the mapper code alone is done by a set and aside from it a big work to do, its incomplete as far as the implementations i was suppose to that needed the refactoring of the entire mapper code to begin with.

@hizzlekizzle
Copy link
Contributor

@LibretroAdmin can they at least give us a list of games they tried that didn't work?

@LibretroAdmin
Copy link
Contributor

LibretroAdmin commented Sep 20, 2024

nothing can be fixed IF no details are provided. if it worked before, then its just a minor fix in the refactor, if its even a code error and not human error. since this was practically "refactored/converted" from my own repo which was already tested (at least for the roms file i have at hand).. the modifications are too many to do this all over again one commit at a time...

you can just revert this repo to what it was and make it a static core, make mine a new repo if you want. but i wont be porting the modifications again one by one. the mapper code alone is done by a set and aside from it a big work to do, its incomplete as far as the implementations i was suppose to that needed the refactoring of the entire mapper code to begin with.

I agree that more info should be provided. I'll try my best again.

I don't really want any breakdown in relationship over this so I'll try my best to see if he can get us something concrete at least that we can work with to improve things.

@negativeExponent
Copy link
Contributor Author

just revert it so people can still use the old core if they want that one. make a separate repo for new fceumm with modifications i can work with or something. i was already woking on new sets to push and this news just make me think its not worth it if the people testing wont even bother giving details. and i dont even trust those CN guys, if theyre the only ones making this isses.

@LibretroAdmin
Copy link
Contributor

just revert it so people can still use the old core if they want that one. make a separate repo for new fceumm with modifications i can work with or something. i was already woking on new sets to push and this news just make me think its not worth it if the people testing wont even bother giving details. and i dont even trust those CN guys, if theyre the only ones making this isses.

Let's not give up motivation yet. I'll keep prodding along to see if I can get some useful information out of him.

Are there any regression tests for mappers so we can determine if changes break something?

@negativeExponent
Copy link
Contributor Author

yeah, more waiting....

@negativeExponent
Copy link
Contributor Author

i have just tested a bunch of multicart based roms, including the newer released ones just a few months ago and everything works, as far as loading the rom itself and selecting a few games.

we should get proper description of the issues instead of making "REVERT THE PR" the defacto when there are issue instead of fixing and moving forward especially when someone is availble to work whichever the kinks are. if its "working before", no reason it will not work, just some kinks to fix due to the conversion from one already about 60% complete repo to here which practially have to compensate for the changes.

if you want this repo to be static, then just revert it back and freeze it for maintenance reason. just be sure whan you want. its always been like this, the waiting causing momentum to break coz we have to listen to reports with missing details.

@negativeExponent
Copy link
Contributor Author

@LibretroAdmin @hunterk @hizzlekizzle

Seriously, you need to revert these changes if im not able to continue to work on this repo. This PR is suppose to be a part of a continues PR i was hoping to be able to do to make it a bit more complete and in sync with my current fork of fceumm. it was not intended to be just 1 PR, even if it works in some places.

Since my PR are getting questioned, not even so much of details then i cant continue working ok it. so much for that "i trust your PR" talk eh.

@LibretroAdmin
Copy link
Contributor

LibretroAdmin commented Sep 22, 2024

Alright @negativeExponent , sorry about all that. I talked to NRS and basically I've come to the conclusion that you can continue with the followup PRs. We will see where this goes. Sorry for the whole detour.

So basically proceed as if this entire convo didn't happen. If someone can actually give me solid feedback on whatever broke then I can present it to you. Until then it won't prevent development from continuing.

@negativeExponent
Copy link
Contributor Author

so your not taking sides and yet the solution is to revert instead of really taking the proper course, and that is to get the info of the problems to resolve. again, if its working before, there is no reason it wont work anymore. since these are the CN guys, i think its more of the "custom hacked" mappers that are specifically modified for specific hacks. i only add roms/mappers i can test with. if someone wants something not there, its can easiliy be added. but i heard enough. this was a waste of time to be honest. again so much for "i trust your PR"

might want to take my rights as well on PX68K as well.

@LibretroAdmin
Copy link
Contributor

LibretroAdmin commented Sep 22, 2024

What are you talking about?

I said just now that nothing needs to be reverted and that you can send any followup PRs you had prepared already.

I was referring to this

This PR is suppose to be a part of a continues PR i was hoping to be able to do to make it a bit more complete and in sync with my current fork of fceumm. it was not intended to be just 1 PR, even if it works in some places.

So I thought you had PRs in waiting. That's what i meant.

What you concluded is the exact opposite of what I just said. So no, what I'm saying is I side with you. Hell, you can even commit it directly if you feel like the PR is taking too long to merge.

So again, nothing needs to be reverted.

instead of really taking the proper course, and that is to get the info of the problems to resolve

No, that is EXACTLY what I decided upon. That's why I said nothing needs to be reverted and you can send the next PRs you had lying in wait. I said before (re-read the last comment) that if there are issues with mappers he needs to file a proper bug report on that mapper and report back before we take action. That's what I said.

So again, there is no problem here. I already agreed to go with your approach.

@LibretroAdmin
Copy link
Contributor

Sent another invite since the previous one had expired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants