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.

Sign up for GitHub

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

Jump to bottom

pushing upstream openbsd support from package. #1330

Closed
wants to merge 3 commits into from
Closed

pushing upstream openbsd support from package. #1330

wants to merge 3 commits into from

Conversation

devnexen
Copy link
Contributor

No description provided.

@MarioLiebisch
Copy link
Member

Is it intentional to use the Android joystick implementation? If so, it might be a good idea to rename or move it.

@devnexen
Copy link
Contributor Author

Yes intentional :-) but it concerns only few files among Android implementations in another hand ... what do you think ?

mantognini
mantognini requested changes Jan 2, 2018
View reviewed changes
Copy link
Member

@mantognini mantognini left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I guess we should add a builder for openbsd then.. :-)

src/SFML/Window/CMakeLists.txt Outdated
elseif(SFML_OS_OPENBSD)
set(PLATFORM_SRC
${PLATFORM_SRC}
${SRCROOT}/Android/JoystickImpl.cpp
Copy link
Member

Choose a reason for hiding this comment

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

Since the android implementation is a dummy one, I'd rather see those files being copied to a new directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will do later in the day fair enough :-)

@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.5.0 via automation Mar 26, 2018
@eXpl0it3r eXpl0it3r added this to the 2.5 milestone Mar 26, 2018
@eXpl0it3r
Copy link
Member

@devnexen We've gotten quite some changes with our CMake setup. Could you resolve the merge conflicts?

@eXpl0it3r eXpl0it3r moved this from Discussion to Requires Adjustments in SFML 2.5.0 Mar 26, 2018
@eXpl0it3r eXpl0it3r moved this from Requires Adjustments to Review & Testing in SFML 2.5.0 Apr 5, 2018
@eXpl0it3r eXpl0it3r moved this from Review & Testing to Ready in SFML 2.5.0 Apr 7, 2018
@binary1248
Copy link
Member

The rebase doesn't look clean. Stuff like INSTALL_MISC_DIR that was removed in ee08e18 are added back in here.

@binary1248
Copy link
Member

I just realized that the only thing preventing us from generalizing FreeBSD support to just "BSD support" is that the Joystick implementations are different (OpenBSD doesn't have one in this PR). Is there a technical reason why OpenBSD can't share the same Joystick implementation with FreeBSD?

@devnexen
Copy link
Contributor Author

devnexen commented Apr 7, 2018

OpenBSD does not have joystick support

@binary1248
Copy link
Member

Browsing through the SDL source code, I noticed that SDL shares the same joystick code among all BSDs. SFML's current FreeBSD Joystick implementation makes use of usbhid instead of the joy driver. usbhid should be available on OpenBSD as well no?

@devnexen
Copy link
Contributor Author

devnexen commented Apr 7, 2018

This code is not compiled for OpenBSD (e.g. SDL_JOYSTICK_USBHID is set to zero in our case)

@binary1248
Copy link
Member

I'm not an expert in OpenBSD by any means, but does this come down to whether usbhid is installed by the user? Or is it not available at all as a package on OpenBSD? There seem to be OpenBSD man pages for usbhid with the remark that usbhid has been available since OpenBSD 3.0.

@devnexen
Copy link
Contributor Author

devnexen commented Apr 7, 2018 •
edited

usbhid is available but no joystick support. The SDL code you mentioned earlier needs joystick.h header which we do not have.

@devnexen
Copy link
Contributor Author

devnexen commented Apr 7, 2018

To come back to SFML until now we used empty android implementation as you can see here https://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/multimedia/sfml/patches/patch-src_SFML_Window_JoystickImpl_hpp?rev=1.2&content-type=text/x-cvsweb-markup

@binary1248
Copy link
Member

OK I guess. 👍

binary1248
binary1248 requested changes Apr 7, 2018
View reviewed changes
cmake/Config.cmake Outdated
@@ -117,3 +121,12 @@ else()
message(FATAL_ERROR "Unsupported compiler")
return()
endif()

# define the install directory for miscellaneous files
Copy link
Member

Choose a reason for hiding this comment

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

This block is no longer used in the current master revision.

include/SFML/Config.hpp Outdated
@@ -85,6 +85,10 @@

// FreeBSD
#define SFML_SYSTEM_FREEBSD
#elif defined(__OpenBSD__)
Copy link
Member

Choose a reason for hiding this comment

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

Empty line missing before this one.

src/SFML/Window/CMakeLists.txt Outdated
@@ -259,7 +265,7 @@ if(SFML_OS_LINUX)
target_link_libraries(sfml-window PRIVATE UDev)
elseif(SFML_OS_WINDOWS)
target_link_libraries(sfml-window PRIVATE winmm gdi32)
elseif(SFML_OS_FREEBSD)
elseif(SFML_OS_FREEBSD OR SFML_OS_OPENBSD)
Copy link
Member

Choose a reason for hiding this comment

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

If we can't interface with joysticks via usbhid do we still need to link to it on OpenBSD?

src/SFML/Window/JoystickImpl.hpp Outdated
@@ -98,7 +98,7 @@ struct JoystickState

#include <SFML/Window/iOS/JoystickImpl.hpp>

#elif defined(SFML_SYSTEM_ANDROID)
#elif defined(SFML_SYSTEM_ANDROID) || defined(SFML_SYSTEM_OPENBSD)
Copy link
Member

Choose a reason for hiding this comment

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

This should be split out and include the OpenBSD implementation instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True that s forgotten change in fact.

@eXpl0it3r eXpl0it3r moved this from Ready to Requires Adjustments in SFML 2.5.0 Apr 7, 2018
Foaly
Foaly reviewed Apr 8, 2018
View reviewed changes
src/SFML/Window/OpenBSD/JoystickImpl.hpp Outdated
namespace priv
{
////////////////////////////////////////////////////////////
/// \brief Android implementation of joysticks
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy pasta :)

@eXpl0it3r eXpl0it3r moved this from Requires Adjustments to Ready in SFML 2.5.0 Apr 8, 2018
@eXpl0it3r
Copy link
Member

Merged in 9da895d

@eXpl0it3r eXpl0it3r closed this Apr 14, 2018
SFML 2.5.0 automation moved this from Ready to Merged / Superseded Apr 14, 2018
@SFML SFML deleted a comment from mantognini Dec 31, 2022
@SFML SFML deleted a comment from mantognini Dec 31, 2022
@SFML SFML deleted a comment from eXpl0it3r Dec 31, 2022
@SFML SFML deleted a comment from eXpl0it3r Dec 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Foaly Foaly Foaly left review comments

@mantognini mantognini mantognini approved these changes

@binary1248 binary1248 binary1248 approved these changes

Assignees
No one assigned
Labels
feature m:config p:linux
Projects
No open projects
SFML 2.5.0
  
Merged / Superseded
Milestone
2.5
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@devnexen @MarioLiebisch @eXpl0it3r @binary1248 @mantognini @Foaly