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

Changes from libretro core adaptation. #376

Closed
wants to merge 39 commits into from

Conversation

zoltanvb
Copy link

Some earlier discussion in #373 .

This is a collection of changes that enables the libretro core to use the same b2 source files as the current standalone version. The actual libretro core adapter code is currently in another repo, but these changes should cause no change to original b2 building.

SDL and UI related functions put behind #ifdef, a few replacement functions from load_save are referenced from elsewhere. By doing this, I could avoid bringing in any dependency during libretro core build.

Static designation removed from certain ROM structures, to be able to refer to them with proper name from libretro side.

strlcpy definition in case of Linux moved behind HAVE_STRLCPY, this may also be useful for b2 as newer glibc has this function and will stop building with conflict.

Few files added to .gitignore.

zoltanvb and others added 27 commits September 28, 2024 19:29
The actual libretro core adapter code is in another repo, but
these changes should cause no change to original b2 building.

SDL and UI related functions put behind #ifdef, a few replacement
functions from load_save are referenced from elsewhere.

Static designation removed from Certain ROM structures, to be able
to refer to them with proper name from libretro side.

strlcpy definition in case of Linux moved behind HAVE_STRLCPY,
this may also be useful for b2 as newer glibc has this function
and will stop building with conflict.
@tom-seddon
Copy link
Owner

tom-seddon commented Oct 23, 2024

Thanks. I still hadn't got too looking at this (just a quick skim to see which files it touched). Having it as a PR will make it easier to work through though. I have some comments and suggestions immediately, which I'll add. I'm hoping this can be done with even fewer source changes at all, though it may require a bit of rearrangement of b2 in places as suggested by this PR.

.gitignore Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a good idea.

src/b2/roms.cpp Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this change for? Should this header do something different?

Copy link
Author

@zoltanvb zoltanvb Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 32-bit Windows versions supported for Libretro, and it is compiled using a cross-compiling docker image set up with older WIN32_WINNT value. Since there should be nothing Windows version dependent in the code that the libretro version uses, this change enables building for those older platforms.

src/b2/misc.cpp Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping the other changes will mean misc.cpp will become unnecessary.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the problem that prompted these changes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of these messages, coming from the cross-compilation environment:

../shared/c/system_windows.cpp: In function 'const char* GetErrorDescription(DWORD)':
../shared/c/system_windows.cpp:153:12: error: narrowing conversion of '-2147205019' from 'SCODE' {aka 'long int'} to 'long unsigned int' [-Wnarrowing]

I have not looked much into it, I must admit.

@tom-seddon
Copy link
Owner

tom-seddon commented Oct 23, 2024

Sorry, a big pile of comments. Requests/suggestions/thoughts welcome!

I think this can be done with no libretro-specific stuff in b2, just with the code arranged a bit better to simplify reuse. This should be better all round as there'll be less to keep on top of and it'll ideally be harder for me to break things. It was always supposed to be this way anyway, but nobody's actually really tested this (aside from the hints of it in the test code), so thanks for doing this and getting it working!

I'd have to do these changes on one of my WIP branches, but it looks like all the libretro code is isolated to src/libretro so the merge should be trouble free and the repercussions of the changes easy to fix up.

I'm afraid this plan comes with no time commitment, but I would like to do this. It'll make the code better generally.

@tom-seddon
Copy link
Owner

tom-seddon commented Oct 23, 2024

BTW, I pushed some changes: https://github.com/tom-seddon/b2/tree/libretro/master - this was me poking about by building on a Linux VM, without trying to run it. I tried to build the WIndows version using MSYS2 MINGW64, but all I got was make: Nothing to be done for 'all'..

Note: this is me experimenting with a view to assessing changes I could make to b2 to make this work! I am not contributing to libretro, sorry, I have to keep my spare time projects scoped!

@tom-seddon tom-seddon mentioned this pull request Oct 23, 2024
6 tasks
@zoltanvb
Copy link
Author

Thanks for taking care! I will go through the items.

@zoltanvb
Copy link
Author

Improvements administered, now amount of changes is less. No worries about timeline, all this work is for making future b2 changes easily transferable, it does not prevent me from refining the libretro side meanwhile.

@tom-seddon
Copy link
Owner

While looking at this stuff, I realised that master had got quite outdated, so I've merged things across and brought it up to date. There will almost certainly be stuff that needs fixing up because of this, sorry about that :( - BBCMicro construction has changed a bit, and the disc interface objects too. Shouldn't be anything too significant, I hope. (Some stuff got moved around to more cleanly separate emulated machine state from transient emulator state that can be derived from it.) Please let me know if anything isn't very clear.

I suggest working from wip/master, which is the branch used to generate prerelease builds and is usually more up to date. (I worry a lot less about merging stuff into it because the resulting build isn't mentioned on the repo front page.) It also includes some of the changes I mentioned above, as tracked by this issue: #379

@zoltanvb
Copy link
Author

On it. But this PR has become contaminated with large amount of other changes I never meant to be upstreamed, so I am closing it now and will reopen in some format.

@zoltanvb zoltanvb closed this Oct 30, 2024
@zoltanvb
Copy link
Author

zoltanvb commented Nov 2, 2024

Changes adapted, the only remaining diffs (to current master) are:

  • DirectDiscImage.c (for the load_save replacement)
  • system.h (HAVE_STRLCPY)
  • system_windows.h, system_windows.cpp (those compilation hacks for the cross-compilation env)

Does the wip/master branch contain new features? I see some references to state handling, that is probably the biggest missing feature also for the libretro core version: emulation states that can be saved/loaded.

@tom-seddon
Copy link
Owner

It could be worth switching to wip/master, as I tend to merge stuff into that fairly promptly, and it does have the strlcpy fix. But everything in wip/master does make its way to master eventually.

I've got the DirectDiscImage stuff hopefully fixed in my wip/tom branch. This will make its way into wip/master fairly soon, once it's had a bit of testing.

@tom-seddon
Copy link
Owner

Regarding system_windows.*, I'm without a Windows PC currently but once I'm up and running again I'll look at the system_windows stuff. But after having a quick look just now:

For the header: I can't remember why I even put that version check in (and the comment from my old repo isn't very illuminating...), so maybe I should just take it out? Looks like it was attempting to prevent building for Windows targets that don't necessarily support WMF, but there's probably a better place for that test than the shared code (and maybe it doesn't actually even need checking for at all).

For the source: I'll put a cast or two in so the code can build as-is without needing to strip anything out. I don't build for Windows using gcc/clang (but see: #385), so not surprising if they barf on something up that VC++ doesn't.

@tom-seddon
Copy link
Owner

7ea2524 removes the windows version checks and adds an explicit cast for the SCODE thing.

Current wip/master also has the disc image derived classes as part of beeb_lib.

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.

2 participants