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

Adds name, manufacturer ID and product ID for joysticks. #528

Merged
merged 1 commit into from Mar 9, 2014

Conversation

Projects
None yet
5 participants
@NoobsArePeople2
Contributor

NoobsArePeople2 commented Feb 2, 2014

Implemented on Windows, Mac OS and Linux as described in ticket #152.

Changes

  1. JoystickCaps renamed to JoystickInfo.
  2. Adds name, manufacturerID and productID properties to JoystickInfo.
  3. Adds getName(unsigned int index), getManufacturerID(unsigned int index) and getProductID(unsigned int index) methods to sf::Joystick.
  4. Linux: Adds dependency on udev for getting joystick manufacturers and product IDs.
  5. Windows: Defines _UNICODE for full Unicode support on Windows. See this and this for details.

Testing

Tested on Windows 8 x64 (compiled with Mingw) with the Xbox 360 pad (wired and wireless) and Dual Shock 3.

Tested on Mac OS 10.7 Lion (compiled with clang) with the Xbox 360 pad (wired), Dual Shock 3, and Wiimote (without attachements, with nunchuck, with classic controller).

Tested on Linux (Ubuntu 13.10, Mint 16, Manjaro, and Fedora 20 all 64-bits) with the Xbox 360 pad (wired). All Linux distros were running in VirtualBox.

@@ -184,6 +200,68 @@ JoystickState JoystickImpl::update()
return state;
}

This comment has been minimized.

@Bromeon

Bromeon Feb 12, 2014

Member

In the following function JoystickImpl::getDeviceName(), there are loads of variables declared before being initialized; you could postpone their declaration until you need them.

@Bromeon

Bromeon Feb 12, 2014

Member

In the following function JoystickImpl::getDeviceName(), there are loads of variables declared before being initialized; you could postpone their declaration until you need them.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Feb 12, 2014

Member

Thanks a lot for the efforts.

I added some remarks in the diff file, sorry for being picky :P

Member

Bromeon commented Feb 12, 2014

Thanks a lot for the efforts.

I added some remarks in the diff file, sorry for being picky :P

@NoobsArePeople2

This comment has been minimized.

Show comment
Hide comment
@NoobsArePeople2

NoobsArePeople2 Feb 12, 2014

Contributor

@Bromeon No worries on the pickiness. All your comments make good points and I'm new to C++ so they're super helpful.

Contributor

NoobsArePeople2 commented Feb 12, 2014

@Bromeon No worries on the pickiness. All your comments make good points and I'm new to C++ so they're super helpful.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Feb 12, 2014

Contributor

There is another thing that I noticed. You use sf::String for m_name everywhere, except for the OSX implementation. There you use a std::string. Is there a reason for that?
That got me wondering: Why does Joystick::getName return a sf::String? Wouldn't a regular std::string be enough?

Contributor

Foaly commented Feb 12, 2014

There is another thing that I noticed. You use sf::String for m_name everywhere, except for the OSX implementation. There you use a std::string. Is there a reason for that?
That got me wondering: Why does Joystick::getName return a sf::String? Wouldn't a regular std::string be enough?

@NoobsArePeople2

This comment has been minimized.

Show comment
Hide comment
@NoobsArePeople2

NoobsArePeople2 Feb 12, 2014

Contributor

@Foaly

...Is there a reason for that?

Oversight. Adding it to the list of things to clean up.

Why does Joystick::getName return a sf::String?

On Windows we're using unicode so when we read the joystick name from the registry it's a std::vector<wchar_t> which corresponds with std::wstring rather than std::string. While we could convert this to std::string it seems to me that sf::String exists to handle these sorts of situations so we should use it instead.

Contributor

NoobsArePeople2 commented Feb 12, 2014

@Foaly

...Is there a reason for that?

Oversight. Adding it to the list of things to clean up.

Why does Joystick::getName return a sf::String?

On Windows we're using unicode so when we read the joystick name from the registry it's a std::vector<wchar_t> which corresponds with std::wstring rather than std::string. While we could convert this to std::string it seems to me that sf::String exists to handle these sorts of situations so we should use it instead.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Feb 13, 2014

Contributor

Of course! That makes sense.

One thing that I find a little confusing are the error messages to sf::err. "Unable to find parent USB device" doesn't seem very descriptive. You should add something like "for reading joystick info". Or maybe remove it completly, because the values all default to zero, so the developer already know that something is wrong, by checking the values. Also the error messages are only in the Unix implementation.

edit: Or where they just left overs from debugging?

Contributor

Foaly commented Feb 13, 2014

Of course! That makes sense.

One thing that I find a little confusing are the error messages to sf::err. "Unable to find parent USB device" doesn't seem very descriptive. You should add something like "for reading joystick info". Or maybe remove it completly, because the values all default to zero, so the developer already know that something is wrong, by checking the values. Also the error messages are only in the Unix implementation.

edit: Or where they just left overs from debugging?

@NoobsArePeople2

This comment has been minimized.

Show comment
Hide comment
@NoobsArePeople2

NoobsArePeople2 Feb 13, 2014

Contributor

Error messages were initially added for debugging but I decided to leave them in. They could go but I think it's helpful to keep them as there are two possible reasons the values may be zero so the different messages can point a dev in the right direction faster. Agree they can be more descriptive.

As for why only in the Unix implementation? Just going with the flow of what's done elsewhere in that implementation: link

Contributor

NoobsArePeople2 commented Feb 13, 2014

Error messages were initially added for debugging but I decided to leave them in. They could go but I think it's helpful to keep them as there are two possible reasons the values may be zero so the different messages can point a dev in the right direction faster. Agree they can be more descriptive.

As for why only in the Unix implementation? Just going with the flow of what's done elsewhere in that implementation: link

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Feb 13, 2014

Member

The existing Unix implementation needs to be cleaned up some time, there are things like

JoystickState JoystickImpl::JoystickImpl::update()

where I wonder how they even compile. Maybe the code is already broken and nobody tested it yet? In any case, focus on the new features, I'll take care of the rest later :)

Error messages are certainly a good idea, when you can make them descriptive. And if it's not too much effort to make them cross-platform, that would be nice.

By the way, it's probably more comfortable for you if suggestions are written in the diff file, isn't it? That way, you directly see what line they refer to, and the comments disappear from this discussion when you commit another revision...

We can also discuss the API of this feature request on the SFML forum.

Member

Bromeon commented Feb 13, 2014

The existing Unix implementation needs to be cleaned up some time, there are things like

JoystickState JoystickImpl::JoystickImpl::update()

where I wonder how they even compile. Maybe the code is already broken and nobody tested it yet? In any case, focus on the new features, I'll take care of the rest later :)

Error messages are certainly a good idea, when you can make them descriptive. And if it's not too much effort to make them cross-platform, that would be nice.

By the way, it's probably more comfortable for you if suggestions are written in the diff file, isn't it? That way, you directly see what line they refer to, and the comments disappear from this discussion when you commit another revision...

We can also discuss the API of this feature request on the SFML forum.

@NoobsArePeople2

This comment has been minimized.

Show comment
Hide comment
@NoobsArePeople2

NoobsArePeople2 Feb 13, 2014

Contributor

Maybe the code is already broken and nobody tested it yet?

I tested it on several distros (see initial message) and it passes a cursory "do I get button press events", etc. tests so it's not completely broken. There may be subtle issues lurking though.

Another issue with joysticks on Linux (at least when running in VirtualBox VM) is that non-joystick devices get identified as joysticks. But like you said, that's a problem for another day.

Adding error messages shouldn't be a problem. Is there an SFML convention for writing error messages?


In some ways having comments at the bottom like this is easier because everything is grouped in a single place but I think it's best to have specific comments in the diff so it's clear what is being discussed.

Contributor

NoobsArePeople2 commented Feb 13, 2014

Maybe the code is already broken and nobody tested it yet?

I tested it on several distros (see initial message) and it passes a cursory "do I get button press events", etc. tests so it's not completely broken. There may be subtle issues lurking though.

Another issue with joysticks on Linux (at least when running in VirtualBox VM) is that non-joystick devices get identified as joysticks. But like you said, that's a problem for another day.

Adding error messages shouldn't be a problem. Is there an SFML convention for writing error messages?


In some ways having comments at the bottom like this is easier because everything is grouped in a single place but I think it's best to have specific comments in the diff so it's clear what is being discussed.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Feb 13, 2014

Member

Adding error messages shouldn't be a problem. Is there an SFML convention for writing error messages?

Not really, just keep it expressive and not overly verbose. If the error occurs because of bad user input, you could state what's wrong. For example, the resource classes (sf::Texture, sf::Font etc.) output errors as follows:

err() << "Failed to create texture, invalid size (" << width << "x" << height << ")" << std::endl;

Btw, quotes can be written after a > character. It also took me quite some time to figure that out ;)

Member

Bromeon commented Feb 13, 2014

Adding error messages shouldn't be a problem. Is there an SFML convention for writing error messages?

Not really, just keep it expressive and not overly verbose. If the error occurs because of bad user input, you could state what's wrong. For example, the resource classes (sf::Texture, sf::Font etc.) output errors as follows:

err() << "Failed to create texture, invalid size (" << width << "x" << height << ")" << std::endl;

Btw, quotes can be written after a > character. It also took me quite some time to figure that out ;)

@NoobsArePeople2

This comment has been minimized.

Show comment
Hide comment
@NoobsArePeople2

NoobsArePeople2 Feb 13, 2014

Contributor

Btw, quotes can be written after a > character. It also took me quite some time to figure that out ;)

Nice!

Contributor

NoobsArePeople2 commented Feb 13, 2014

Btw, quotes can be written after a > character. It also took me quite some time to figure that out ;)

Nice!

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Feb 14, 2014

Contributor

Alright that sounds great!

Looking at it again, I noticed something else, On Mac and Windows the code that actually reads the data is encapsulated in seperate, private methods (for example getDeviceName). On Linux everything is in the open method. I think it would be better to have it consistent on all platforms.

Contributor

Foaly commented Feb 14, 2014

Alright that sounds great!

Looking at it again, I noticed something else, On Mac and Windows the code that actually reads the data is encapsulated in seperate, private methods (for example getDeviceName). On Linux everything is in the open method. I think it would be better to have it consistent on all platforms.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 2, 2014

Member

The PR is not mergeable anymore because of a whitespace change in JoystickImpl.cpp.

A few last remarks:

  • Please move the Joystick::Identification constructor to the .cpp file (you can insert it at the end)
  • Is there a specific reason why JoystickCaps is renamed to JoystickCapabilities? I think we should change as few of the previous implementation as possible.
  • The member variable JoystickManager::Item::id should be called identification like everywhere else
  • In the OSX implementation, you should not use uint32_t (C++11 or C) but rather Apple's SInt32 (see here)
Member

Bromeon commented Mar 2, 2014

The PR is not mergeable anymore because of a whitespace change in JoystickImpl.cpp.

A few last remarks:

  • Please move the Joystick::Identification constructor to the .cpp file (you can insert it at the end)
  • Is there a specific reason why JoystickCaps is renamed to JoystickCapabilities? I think we should change as few of the previous implementation as possible.
  • The member variable JoystickManager::Item::id should be called identification like everywhere else
  • In the OSX implementation, you should not use uint32_t (C++11 or C) but rather Apple's SInt32 (see here)
@NoobsArePeople2

This comment has been minimized.

Show comment
Hide comment
@NoobsArePeople2

NoobsArePeople2 Mar 3, 2014

Contributor

The PR is not mergeable anymore because of a whitespace change in JoystickImpl.cpp.

The Unix implementation? If so, it's fixed in the latest commit. I also changed a bunch of tabs to spaces in the FreeBSD implementation a few commits back.

Please move the Joystick::Identification constructor to the .cpp file (you can insert it at the end)

Can you show me an example of what this would look like as I'm not sure how to go about it?


Your other changes have been made. For now I've left manufacturerId as-is (instead of changing it to vendorId as discussed in the forum). I think I'll change it when I make the Joystick::Identification constructor change unless you have objections.

Contributor

NoobsArePeople2 commented Mar 3, 2014

The PR is not mergeable anymore because of a whitespace change in JoystickImpl.cpp.

The Unix implementation? If so, it's fixed in the latest commit. I also changed a bunch of tabs to spaces in the FreeBSD implementation a few commits back.

Please move the Joystick::Identification constructor to the .cpp file (you can insert it at the end)

Can you show me an example of what this would look like as I'm not sure how to go about it?


Your other changes have been made. For now I've left manufacturerId as-is (instead of changing it to vendorId as discussed in the forum). I think I'll change it when I make the Joystick::Identification constructor change unless you have objections.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 3, 2014

Member

The Unix implementation? If so, it's fixed in the latest commit.

JoystickImpl.cpp in both Unix and FreeBSD. The commit e1c40c9 led to the incompatibility. Tell me when you need help at resolving the conflicts, but I think it shouldn't be too difficult.

Can you show me an example of what this would look like as I'm not sure how to go about it?

Sure! In the header (Joystick.hpp):

    struct Identification
    {
        Identification();

        sf::String   name;               ///< Name of the joystick
        unsigned int vendorId;           ///< Manufacturer identifier
        unsigned int productId;          ///< Product identifier
    };

At the end of the implementation file (Joystick.cpp):

Joystick::Identification::Identification() :
name     ("No Joystick"),
vendorId (0),
productId(0)
{

}

For now I've left manufacturerId as-is (instead of changing it to vendorId as discussed in the forum). I think I'll change it when I make the Joystick::Identification constructor change unless you have objections.

Yes, we should change it to vendorId.

Member

Bromeon commented Mar 3, 2014

The Unix implementation? If so, it's fixed in the latest commit.

JoystickImpl.cpp in both Unix and FreeBSD. The commit e1c40c9 led to the incompatibility. Tell me when you need help at resolving the conflicts, but I think it shouldn't be too difficult.

Can you show me an example of what this would look like as I'm not sure how to go about it?

Sure! In the header (Joystick.hpp):

    struct Identification
    {
        Identification();

        sf::String   name;               ///< Name of the joystick
        unsigned int vendorId;           ///< Manufacturer identifier
        unsigned int productId;          ///< Product identifier
    };

At the end of the implementation file (Joystick.cpp):

Joystick::Identification::Identification() :
name     ("No Joystick"),
vendorId (0),
productId(0)
{

}

For now I've left manufacturerId as-is (instead of changing it to vendorId as discussed in the forum). I think I'll change it when I make the Joystick::Identification constructor change unless you have objections.

Yes, we should change it to vendorId.

@NoobsArePeople2

This comment has been minimized.

Show comment
Hide comment
@NoobsArePeople2

NoobsArePeople2 Mar 3, 2014

Contributor

I just pushed up the vendorId and Identification constructor changes. On the Identification change: what's the reasoning on putting the constructor in the cpp file? This might be my noobiness showing, but I haven't seen that done before.

Tell me when you need help at resolving the conflicts, but I think it shouldn't be too difficult.

How do you want to do this? Typically I rebase the changes from upstream and manually resolve any merge conflicts when I need to pull in changes to a branch.

Contributor

NoobsArePeople2 commented Mar 3, 2014

I just pushed up the vendorId and Identification constructor changes. On the Identification change: what's the reasoning on putting the constructor in the cpp file? This might be my noobiness showing, but I haven't seen that done before.

Tell me when you need help at resolving the conflicts, but I think it shouldn't be too difficult.

How do you want to do this? Typically I rebase the changes from upstream and manually resolve any merge conflicts when I need to pull in changes to a branch.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 3, 2014

Member

The same reasons as for every other function and every other constructor: fewer implementation details in the header, client need not recompile upon changes, no need to include further dependencies in the header, shorter compile time. Here those points are not crucial, but it's good to do it consistently.

Yes, I would also have to manually resolve the conflicts. You should try to rebase on the SFML master commit.

Member

Bromeon commented Mar 3, 2014

The same reasons as for every other function and every other constructor: fewer implementation details in the header, client need not recompile upon changes, no need to include further dependencies in the header, shorter compile time. Here those points are not crucial, but it's good to do it consistently.

Yes, I would also have to manually resolve the conflicts. You should try to rebase on the SFML master commit.

@NoobsArePeople2

This comment has been minimized.

Show comment
Hide comment
@NoobsArePeople2

NoobsArePeople2 Mar 3, 2014

Contributor

Yes, I would also have to manually resolve the conflicts. You should try to rebase on the SFML master commit.

Cool, I'll do that this evening.

Contributor

NoobsArePeople2 commented Mar 3, 2014

Yes, I would also have to manually resolve the conflicts. You should try to rebase on the SFML master commit.

Cool, I'll do that this evening.

@NoobsArePeople2

This comment has been minimized.

Show comment
Hide comment
@NoobsArePeople2

NoobsArePeople2 Mar 4, 2014

Contributor

Hmmmm...did the rebase on a new branch then merged it back into the PR. Looks like a mess after pushing to github. Thoughts on the best way to clean it up? I don't have a ton of experience with PRs.

Contributor

NoobsArePeople2 commented Mar 4, 2014

Hmmmm...did the rebase on a new branch then merged it back into the PR. Looks like a mess after pushing to github. Thoughts on the best way to clean it up? I don't have a ton of experience with PRs.

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Mar 4, 2014

Don't rebase and merge. Rebase and force push instead.

retep998 commented Mar 4, 2014

Don't rebase and merge. Rebase and force push instead.

@NoobsArePeople2

This comment has been minimized.

Show comment
Hide comment
@NoobsArePeople2

NoobsArePeople2 Mar 4, 2014

Contributor

Don't rebase and merge. Rebase and force push instead.

Is this advice for the current situation or future reference?

Contributor

NoobsArePeople2 commented Mar 4, 2014

Don't rebase and merge. Rebase and force push instead.

Is this advice for the current situation or future reference?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 4, 2014

Member

In the end it would be fine if you could do an interactive rebase and squash everything to a single commit, so that we have a clean history when merging it to SFML-master.

  1. Switch to the branch where your changes are still linear, before merging
  2. Write git rebase -i xy where xy is the hash of the last SFML commit before you started
  3. In the document that will be opened, change "pick" of all your commits except the first one to "squash" or "s" (squash = combine with previous commit)
  4. Close the document and wait
  5. Check that there's only a single commit left that contains all your changes
  6. Push the changes with the -f flag to rewrite history (this is called force-pushing)

Don't hesitate to ask if things are unclear. If you're doing it for the first time, it might be worth making a backup before ;) otherwise there is git reflog as a safety net.

Member

Bromeon commented Mar 4, 2014

In the end it would be fine if you could do an interactive rebase and squash everything to a single commit, so that we have a clean history when merging it to SFML-master.

  1. Switch to the branch where your changes are still linear, before merging
  2. Write git rebase -i xy where xy is the hash of the last SFML commit before you started
  3. In the document that will be opened, change "pick" of all your commits except the first one to "squash" or "s" (squash = combine with previous commit)
  4. Close the document and wait
  5. Check that there's only a single commit left that contains all your changes
  6. Push the changes with the -f flag to rewrite history (this is called force-pushing)

Don't hesitate to ask if things are unclear. If you're doing it for the first time, it might be worth making a backup before ;) otherwise there is git reflog as a safety net.

Adds name, vendor ID and product ID for joysticks.
- Implemented on Windows, Mac OS and Linux.
- Adds sf::Joystick::Identification structure to hold
  name, vendor ID and product ID.
@NoobsArePeople2

This comment has been minimized.

Show comment
Hide comment
@NoobsArePeople2

NoobsArePeople2 Mar 4, 2014

Contributor

Okay, I think I've got it now. Let me know if everything can merge cleanly.

In the future would it be best to squash all my commits before rebasing in upstream changes? I think part of the problem is that I had a bunch of upstream commits interspersed with my commits.

Contributor

NoobsArePeople2 commented Mar 4, 2014

Okay, I think I've got it now. Let me know if everything can merge cleanly.

In the future would it be best to squash all my commits before rebasing in upstream changes? I think part of the problem is that I had a bunch of upstream commits interspersed with my commits.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 4, 2014

Member

Thanks a lot. From a first glance it looks good, and the GitHub merge button is green again ;)
I'll let you know in case of issues.

Member

Bromeon commented Mar 4, 2014

Thanks a lot. From a first glance it looks good, and the GitHub merge button is green again ;)
I'll let you know in case of issues.

Bromeon added a commit that referenced this pull request Mar 9, 2014

Merge pull request #528 from NoobsArePeople2/joystick
Adds name, manufacturer ID and product ID for joysticks.

@Bromeon Bromeon merged commit 6b2a4c2 into SFML:master Mar 9, 2014

@Bromeon Bromeon added this to the 2.2 milestone Mar 26, 2014

@Bromeon Bromeon self-assigned this Mar 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment