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

Update / Port to SFML 2.1 #1522

Merged
merged 2 commits into from Nov 26, 2014
Merged

Update / Port to SFML 2.1 #1522

merged 2 commits into from Nov 26, 2014

Conversation

jcowgill
Copy link
Contributor

@jcowgill jcowgill commented Nov 9, 2014

This PR updates dolphin to use SFML 2.1. It was based on the patch by Brandon Barnes / WinterKnight from here: https://forums.dolphin-emu.org/Thread-patch-use-sfml-2-1.

I've mainly done this PR for the debian package since debian only ships SFML 2.1. I could just apply this PR to the debian package, but I'd prefer it if there were only minimal differences between the my package and the official version.

I've split this into 2 commits: one which updates everything in Externals/ and the other which patches the source code, but I can squash them if you prefer.

Most of the changes were just function renames (mainly capitalization), but some changes were bigger since parts of the SFML network api were redesigned:

  • sf::TcpSocket is no longer copy constructable. Unfortunately SFML is not a C++11 library so I've had to use unique_ptrs to handle the places where sockets are moved around.
  • sf::SocketSelector has an "isReady" method instead of a "GetSocketReady" now. This means you have to iterate over all the connected sockets to find which one is ready in NetPlayServer. This also meant that the std::map of players had no more users - so I changed it to a std::list.

@Parlane
Copy link
Member

Parlane commented Nov 9, 2014

Two commits here is good practise.

@Parlane
Copy link
Member

Parlane commented Nov 9, 2014

@dolphin-emu-bot rebuild

@jcowgill
Copy link
Contributor Author

Grr - that should teach me to actually purge SFML from my system when testing the bundled copy.
The build should work now.

@Parlane
Copy link
Member

Parlane commented Nov 10, 2014

@dolphin-emu-bot rebuild

@Parlane
Copy link
Member

Parlane commented Nov 10, 2014

You are failing our lint step. Includes must be in alphabetical order. https://buildbot.dolphin-emu.org/builders/lint/builds/2227/steps/shell/logs/stdio

@Parlane
Copy link
Member

Parlane commented Nov 10, 2014

@dolphin-emu-bot rebuild

@Parlane
Copy link
Member

Parlane commented Nov 10, 2014

Something else is up with the buildbot sorry. I'll look at what is causing the failure tomorrow.

@waddlesplash
Copy link
Contributor

-1 from me and @Sonicadvance1. The SFML dependency is really stupid, we should just write our own socket abstraction layer. I volunteer to do this, if PPSSPP doesn't have one already.

@degasus
Copy link
Member

degasus commented Nov 10, 2014

I agree, SFML should be removed from the project, but as long as this has not happend, what's wrong with updating the library?

@waddlesplash
Copy link
Contributor

Because it'll add a lot of unnecessary bulk to the Git repo? I should have enough time between today and Wednesday to do the needed work to remove it, is that soon enough?

@lioncash
Copy link
Member

Your PR will also need to be reviewed and shown to be on equal standing with the SFML code. So, along with the abstracted socket layer, you should also be writing unit tests for this replacement as well.

@waddlesplash
Copy link
Contributor

Err. On second thought, moving away from SFML is going to be really hard because DolphinWX uses the HTTP/FTP stuff. 😢 I suppose moving away from SFML ain't gonna happen until after we nuke DolphinWX then...

At least delete the FTP classes then (they're in their own files), we shouldn't need those.

After that, +1 from me, I guess.

@jcowgill
Copy link
Contributor Author

I removed the FTP classes. The buildbot seemed to be complaining about line endings so I changed them all to unix style - hopefully that will help.

@waddlesplash
Copy link
Contributor

Build succeeded everywhere but Mac & Win, and those fail because it has an outdated Git or something (patch fails). LGTM, but can someone test manually on Windows and Mac?

@skidau
Copy link
Contributor

skidau commented Nov 23, 2014

@dolphin-emu-bot rebuild

@@ -50,6 +50,7 @@
<PreprocessorDefinitions>_CRT_SECURE_NO_WARNINGS;_CRT_SECURE_NO_DEPRECATE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
<PreprocessorDefinitions>USE_UPNP;%(PreprocessorDefinitions)</PreprocessorDefinitions>
<PreprocessorDefinitions>PSAPI_VERSION=1;_M_X86=1;%(PreprocessorDefinitions)</PreprocessorDefinitions>
<PreprocessorDefinitions>SFML_STATIC;%(PreprocessorDefinitions)</PreprocessorDefinitions>

This comment was marked as off-topic.

This comment was marked as off-topic.

@shuffle2
Copy link
Contributor

The patch appears to be failing because something is case-sensitive. Should be easy to fix @jcowgill ...

@jcowgill
Copy link
Contributor Author

I think it fails because the case of the IpAddress.* files changed and git doesn't like it. I'll change it back - that should be OK as long as noone ever incudes IpAddress.hpp manually.

- all files converted to unix line endings
- files from other subsystems and most of system have been removed
- include/SFML/System.hpp and include/SFML/Network.hpp modified to
  not include removed stuff
- IpAddress.*pp renamed to IPAddress.*pp to workaround git apply bug
  with case-only renames
skidau added a commit that referenced this pull request Nov 26, 2014
@skidau skidau merged commit b806680 into dolphin-emu:master Nov 26, 2014
@Icekhaos
Copy link

This leads us to: What is the status of Qt vs Wx?

@lioncash
Copy link
Member

The Qt UI at the moment is still very early in development. At most, it has a game list and can boot games, that's about it.

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