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

Added joystick example. #1363

Merged
merged 1 commit into from Apr 5, 2018
Merged

Added joystick example. #1363

merged 1 commit into from Apr 5, 2018

Conversation

@binary1248
Copy link
Member

binary1248 commented Feb 3, 2018

Originally used when testing #1326 but can be used with the existing code as well.

@binary1248 binary1248 self-assigned this Feb 3, 2018
@eXpl0it3r eXpl0it3r added the s:accepted label Feb 3, 2018
@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.5.0 via automation Feb 3, 2018
@binary1248 binary1248 force-pushed the feature/joystick_example branch from b628853 to 58850ee Feb 3, 2018
window.clear();

// Draw the label-value sf::Text objects
for (Texts::const_iterator i = texts.begin(); i != texts.end(); ++i)

This comment has been minimized.

Copy link
@MarioLiebisch

MarioLiebisch Feb 4, 2018

Member

If there's no controller connected it will still spit out basically placeholder text for the first controller. Maybe there should be some instruction like "No controllers could be found. Connect a gamepad or joystick to test it."?

This comment has been minimized.

Copy link
@binary1248

binary1248 Feb 5, 2018

Author Member

The text that is always output isn't placeholder text, they are simply the labels of the joystick items. When a joystick is connected, values will be displayed for the last joystick whose state was modified. When no joysticks are connected, no values are displayed.

This comment has been minimized.

Copy link
@MarioLiebisch

MarioLiebisch Feb 5, 2018

Member

Yeah, but (at least to me) from a first look it's still not clear whether the program is stuck or just couldn't find any joystick. So maybe just rather than setting ID to nothing, set it to <Not connected>, if there's no controller? Similar not existing buttons/axes should possibly output "n/a" rather than nothing.

@MarioLiebisch

This comment has been minimized.

Copy link
Member

MarioLiebisch commented Feb 4, 2018

I'm not really sure about the whole text output only. I'd prefer something visual, even if it just consists of simple shapes. Maybe something like this (potentially with more text):
Imgur

@binary1248

This comment has been minimized.

Copy link
Member Author

binary1248 commented Feb 5, 2018

The point of this example is to demonstrate usage of the joystick API. The predecessor to this code would output purely to the console, however it would become unreadable when outputting all the possible values of a joystick's state hence the sfml-graphics output. The current text rendering already takes up almost half of the code. Making it any more sophisticated such as in the picture you posted will shift the focus of the code away from the joystick API to the drawable API which shouldn't be the case in a joystick example. This should stay as an example and not become some kind of debug tool.

@MarioLiebisch

This comment has been minimized.

Copy link
Member

MarioLiebisch commented Feb 5, 2018

Yeah, probably true,

text.second.setPosition(80.f, 5.f + ((sf::Joystick::AxisCount + i + 2) * font.getLineSpacing(14)));
}

for (Texts::iterator i = texts.begin(); i != texts.end(); ++i)

This comment has been minimized.

Copy link
@MarioLiebisch

MarioLiebisch Feb 5, 2018

Member

Also, maybe just me, but I prefer to reserve i, j, etc. for numerical indices, while using a, b, etc. for iterators (even though [i]terator makes sense).

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Feb 5, 2018

Member

My personal convention, and thus most likely what's used in SFML, is to use it for iterators.


namespace
{
typedef std::map<std::string, std::pair<sf::Text, sf::Text> > Texts;

This comment has been minimized.

Copy link
@MarioLiebisch

MarioLiebisch Feb 5, 2018

Member

While convenient and using existing classes, I'd really just define a small struct for the values here, even if it's just to identify the components as label and value or something like that. It's about readability and the chained expressions with first or second just feel a bit odd.

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Feb 5, 2018

Member

Agreed. std::pair is never a good choice.

std::ostringstream sstr;

// Axes labels in as C strings
const char* axislabels[] = {"X", "Y", "Z", "R", "U", "V", "PovX", "PovY"};

This comment has been minimized.

Copy link
@MarioLiebisch

MarioLiebisch Feb 5, 2018

Member

While it's unlikely the enum values for the axes might change, I'm not really a fan of just assuming a constant/fixed order here.

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Feb 5, 2018

Member

In core SFML code I would agree, but this is just an example, so it's a good thing if we keep this kind of boilerplate code as short as possible.

@MarioLiebisch

This comment has been minimized.

Copy link
Member

MarioLiebisch commented Feb 5, 2018

As an added feature, maybe allow the user to increase/decrease the movement threshold with some keys, like Page Up/Down, visualizing what it does?

@eXpl0it3r eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.5.0 Feb 10, 2018
@eXpl0it3r eXpl0it3r moved this from Review & Testing to Ready in SFML 2.5.0 Feb 23, 2018
@eXpl0it3r eXpl0it3r force-pushed the feature/joystick_example branch from 58850ee to 2b8dede Mar 23, 2018
@eXpl0it3r

This comment has been minimized.

Copy link
Member

eXpl0it3r commented Mar 23, 2018

There are still some open suggestions, are you going to change things @binary1248? I mean it's just an example, doesn't have to be the best thing ever.

@binary1248

This comment has been minimized.

Copy link
Member Author

binary1248 commented Mar 23, 2018

I still intend to work on this... yes.

@eXpl0it3r eXpl0it3r moved this from Ready to Requires Adjustments in SFML 2.5.0 Mar 23, 2018
@binary1248 binary1248 force-pushed the feature/joystick_example branch from 2b8dede to 1ca7f23 Mar 24, 2018
@binary1248

This comment has been minimized.

Copy link
Member Author

binary1248 commented Mar 24, 2018

@MarioLiebisch, @LaurentGomila, @eXpl0it3r: All the mentioned issues should be resolved in the current version.

@eXpl0it3r eXpl0it3r moved this from Requires Adjustments to Review & Testing in SFML 2.5.0 Mar 26, 2018
@eXpl0it3r

This comment has been minimized.

Copy link
Member

eXpl0it3r commented Mar 26, 2018

I don't own a joystick, can someone test this?

@binary1248 binary1248 force-pushed the feature/joystick_example branch from 1ca7f23 to a786ca1 Mar 26, 2018
@danieljpetersen

This comment has been minimized.

Copy link

danieljpetersen commented Mar 28, 2018

I just tried this out on Linux Mint 18.3

  • The Steam Controller identified as a 360 controller. It doesn't seem like this is working correctly here. Only three buttons have an effect -- The threshold value goes up when you press the X button, and it goes down when you press the Y button. Pressing the start button causes the program to exit, which I guess is to be expected.
  • Everything works perfectly with the Logitech Extreme 3D (this thing)
  • The PC version of the 360 controller also works perfectly for me.
@eXpl0it3r

This comment has been minimized.

Copy link
Member

eXpl0it3r commented Mar 28, 2018

Thanks for testing!

The Steam Controller identified as a 360 controller. It doesn't seem like this is working correctly here. Only three buttons have an effect -- The threshold value goes up when you press the X button, and it goes down when you press the Y button. Pressing the start button causes the program to exit, which I guess is to be expected.

Sounds more like a potential driver issue. If you have time to track down the issue, maybe there's something we need to adjust on our side?

@danieljpetersen

This comment has been minimized.

Copy link

danieljpetersen commented Mar 28, 2018

Perhaps it is a driver issue. Apparently the Steam Controller isn't mainlined into the Linux kernel yet, and you need to have Steam running in the background order for the controller to properly work.

I'll test it out on Windows.

@eXpl0it3r eXpl0it3r moved this from Review & Testing to Ready in SFML 2.5.0 Mar 28, 2018
@danieljpetersen

This comment has been minimized.

Copy link

danieljpetersen commented Mar 28, 2018

Hm. I get the same result in Windows, except this time it's not reporting that any controller is connected. I still think you're right though, it looks like the controller is mapped to keyboard keys and not presenting itself as a joystick?

@binary1248

This comment has been minimized.

Copy link
Member Author

binary1248 commented Mar 28, 2018

This PR doesn't change the way joysticks are handled by SFML. It is merely an example of how to use the API. If wrong values are displayed, then any application making use of the SFML joystick implementation will also function incorrectly. As such, if you find and are able to reproduce any bugs in the current joystick implementation, you should discuss the issue on the forum and possibly open an issue against the actual SFML joystick implementation.

@eliasdaler

This comment has been minimized.

Copy link
Contributor

eliasdaler commented Mar 29, 2018

Logitech Gamepad F310 on Linux - everything works as expected in DirectInput and XInput modes.

Copy link
Member

mantognini left a comment

Overall, it looks good and works nicely on macOS.


typedef std::map<std::string, JoystickObject> Texts;
Texts texts;
std::ostringstream sstr;

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 2, 2018

Member

What is the motivation for this global ss?

This comment has been minimized.

Copy link
@binary1248

binary1248 Apr 3, 2018

Author Member

Having to construct/destroy the ss everywhere where it is currently used would cause unnecessary overhead. I would have liked to use std::to_string but we aren't there yet. 😉

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Apr 3, 2018

Member

Having to construct/destroy the ss everywhere where it is currently used would cause unnecessary overhead.

Is it something that you measured? I've always seen people instantiating (or recommending to instantiate) this locally, only where it is needed.

This comment has been minimized.

Copy link
@binary1248

binary1248 Apr 3, 2018

Author Member

I always use stringstreams to stringify numerical values, also when getting a rough feel for performance (frame times) as you know. I noticed a significant delta when recreating them constantly, which is why I made it a habit to just resort to using long-lived ones. It is much faster to just empty the buffer and re-fill it constantly than always have to construct/destruct the object including its internal state etc.

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 4, 2018

Member

I don't know about the perf cost, never had to measure it. I guess one could always encapsulate this using a static variable in a function. Not saying you should do it, just a random thought.

// Update threshold if the user wants to change it
float newThreshold = threshold;

if (sf::Keyboard::isKeyPressed(sf::Keyboard::PageUp))

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 2, 2018

Member

I don't have this key on my keyboard. Up seems more suitable.

if (sf::Keyboard::isKeyPressed(sf::Keyboard::PageUp))
newThreshold += 0.1f;

if (sf::Keyboard::isKeyPressed(sf::Keyboard::PageDown))

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 2, 2018

Member

Idem with Down.

@binary1248 binary1248 force-pushed the feature/joystick_example branch from a786ca1 to 1ca226d Apr 3, 2018
@binary1248

This comment has been minimized.

Copy link
Member Author

binary1248 commented Apr 3, 2018

@mantognini Fixed.

@binary1248 binary1248 requested review from mantognini and MarioLiebisch Apr 4, 2018
@eXpl0it3r

This comment has been minimized.

Copy link
Member

eXpl0it3r commented Apr 5, 2018

There's a small conflict due to the iOS Demo being merged.

@eXpl0it3r eXpl0it3r moved this from Ready to Requires Adjustments in SFML 2.5.0 Apr 5, 2018
@binary1248 binary1248 force-pushed the feature/joystick_example branch from 1ca226d to fe1407b Apr 5, 2018
@binary1248

This comment has been minimized.

Copy link
Member Author

binary1248 commented Apr 5, 2018

@eXpl0it3r Fixed.

@eXpl0it3r eXpl0it3r moved this from Requires Adjustments to Ready in SFML 2.5.0 Apr 5, 2018
@eXpl0it3r eXpl0it3r merged commit fe1407b into master Apr 5, 2018
15 of 16 checks passed
15 of 16 checks passed
ios-clang-el-capitan Build #98 done.
Details
android-armeabi-v7a-api13 Build #94 done.
Details
debian-gcc-64 Build #98 done.
Details
freebsd-gcc-64 Build #92 done.
Details
osx-clang-el-capitan Build #91 done.
Details
static-analysis Build #96 done.
Details
windows-gcc-492-tdm-32 Build #89 done.
Details
windows-gcc-492-tdm-64 Build #90 done.
Details
windows-gcc-630-mingw-32 Build #88 done.
Details
windows-gcc-630-mingw-64 Build #84 done.
Details
windows-vc11-32 Build #80 done.
Details
windows-vc11-64 Build #77 done.
Details
windows-vc12-32 Build #77 done.
Details
windows-vc12-64 Build #77 done.
Details
windows-vc14-32 Build #77 done.
Details
windows-vc14-64 Build #77 done.
Details
SFML 2.5.0 automation moved this from Ready to Merged / Superseded Apr 5, 2018
@eXpl0it3r eXpl0it3r deleted the feature/joystick_example branch Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
SFML 2.5.0
  
Merged / Superseded
7 participants
You can’t perform that action at this time.