Added support for 3Dconnexion Space Navigator #26
Conversation
|
Hi Jinrong, Thanks for this patch. We're in the middle of getting a release out, but will examine it shortly after. Thanks and merry Christmas, |
|
Is there anybody taking care of these pull request? |
|
yes sorry, we're examining it now |
| if (res == -1) | ||
| return; | ||
|
|
||
| rawInputBuffer = (BYTE*)malloc(size); |
There was a problem hiding this comment.
Never cast the result of malloc, terrible habit, hides forgetting to include stdlib.h which will lead to crashes on systems where sizeof(int) != sizeof(void*)
There was a problem hiding this comment.
Thanks for pointing this out. I have noticed that fg_spaceball_mswin.c includes "../fg_internal.h" which already includes stdlib.h at line 104. For safety I added the stdlib.h in the fg_spaceball_mswin.c. Please help check the latest commit. Thanks!
|
The _WIN32_WINNT macro seems to be concerned with Windows version number. What version is required for this patch to work? 0x0501 for Windows XP? 0x0600 for Windows Vista? I don't think it's reasonable to break XP or Vista support without the broader FreeGLUT crowd deciding to. An alternative is to ifdef this feature according to the required _WIN32_WINNT version. |
|
His initial comment discusses the _WIN32_WINNT macro, though I don't completely follow what the macro is being used for, or if CMake is just inserting it automatically. Looking at the code, all structures and functions that are used are, according to MSDN, supported in Windows XP. |
|
The relevant MSDN documentation is here: |
|
So, the CMakeList file defines _WIN32_WINNT=0x0500, which is Windows 2000. All the functionality for this pull request is XP and higher (which WINNT version, not sure). This explains why @stonexjr said it was getting disabled. Perhaps the macro should only be defined, as indicated in the CMakeList file, when an older compiler like MSVC6 is used. That, plus making sure the code is otherwise disabled for MSVC6, and it could work. |
|
Seems to me a "XP or newer" baseline is a reasonable step. I think raising this on the mail list would be appropriate. See: |
|
Oh, it was added for MSVC6, indeed. (Looking at the git blame) |
|
Hi, yes, most of freeglut still runs on older platforms and compiles with
older compilers (MSVC6 indeed, John F's favorite). If a simple ifdef (not a
hairy-mess of ifdefs) can keep it that way, I think we should. If it is not
that simple, now would indeed be a good way to discuss it on the mailing
list.
|
|
It does appear that cmake can conditionalize the defines based on the target toolchain. (VC6 etc) |
|
@stonexjr I think the only thing that may be holding this up now is _WIN32_WINNT. If you look at the last couple messages @nigels-com, @dcnieho, and myself discussed (as far as I can tell): Does that make sense? If so, that may be the last needed thing to accept the pull request. |
|
The comments need to be changed from C++/C99 end-of-line style to C89 block comments. Other than that, I don't see any issues in merging this. It doesn't break anything GNU/linux-side. |
|
@stonejxr, It would be great if you can shed further light on the _WIN32_WINNT macro issue. What minimum setting do you need? Maybe the simplest way to deal with this to ifdef the code on availability of WM_INPUT, similar to line 1500 of fg_main_mswin.c in current head. Would that be a solution without all the CMake trickery, guys? |
|
@dcnieho Well, from looking at his code, all everything that he uses is XP and higher. For |
|
Ah good! Then for just about anyone things would work. But my point is that On Mon, May 25, 2015 at 6:58 PM, rcmaniac25 notifications@github.com
|
|
@dcnieho, The minimum setting for this patch is making sure _WIN32_WINNT >= 0x0501, so windows XP or higher is supported, since WM_INPUT is available if _WIN32_WINNT>=0x0501, according to WinUser.h. |
|
@rcmaniac25, Current CMake file would disable this patch due to the _WIN32_WINNT it defines is 0x0500. I will go ahead and fix this issue based on your advice and some comments format (@jtsiomb). Thanks for all of your suggestions @dcnieho, @nigels-com, @jtsiomb. |
|
On Mon, May 25, 2015 at 9:15 PM, Jinrong Xie notifications@github.com
That is interesting. Then our switch over to CMake has also killed touch Thanks for fixing this up! |
|
@dcnieho, you're right. The touch support is disabled on my win8 device. |
…NVER definitions in visual studio project setting.
|
Hi all, please help review the new commit. I have basically changed the CMake file to check against MSVC_VERSION and definite appropriate _WIN32_WINNT/WINVER accordingly. Also, I have put #ifdef macro in the source code to ensures the needed functionality is available, |
|
@stonexjr, ok! I think we should bring up the _WIN32_WINNT/WINVER issue on the mailing list. Your commit is a good starting point for that... |
|
@dcnieho The CMake stuff was my suggestion, which looks good to me. |
| ADD_DEFINITIONS(-DWINVER=0x0500) | ||
| ENDIF() | ||
|
|
||
| IF(NOT(MSVC_VERSION LESS "1600")) |
There was a problem hiding this comment.
does this overwrite the earlier define above? should these additions be one big if, elseif, else block?
There was a problem hiding this comment.
Nice catch. Actually it doesn't overwrite earlier definition. I have changed it to if, elseif, else block.
There was a problem hiding this comment.
|
If my above concern is not a concern ( ;) ), i propose to merge this first and then discuss further. As we have managed to ship a fg 3.0 with permanently disabled touch support on windows, its important to get a new release out soon. |
|
On Tue, May 26, 2015 at 02:59:50PM -0700, Diederick C. Niehorster wrote:
To be fair, who the hell uses windows on touch-based devices? And of John Tsiombikas |
|
@dcnieho Had my own delay in responding, I think it's good now. The think the touch concern would also be addressed too. Though in terms of general touch support (I think I said this somewhere, but don't remember where), Windows 8 replaced how touch input works, so if we have something that supports touch input on Windows, it won't work on Windows 8 and newer. But that's for a different ticket in my opinion. |
Added support for 3Dconnexion Space Navigator
|
@stonexjr Sorry that this took so long and thanks a lot for the hard work! @rcmaniac25 Thanks! The Windows 8 touch issue is indeed something else, I'll put it on the todo list on the website. |
|
@dcnieho It's been great experience working with you folks! And thanks all for your constructive suggestion. |
|
@stonexjr: I found what I think is a bug: the implementation on Windows doesn't indicate what button was pressed. Based on what I can tell from the X11 implementation, the SpaceButton callback takes a button and a button state. On X11, button and button state provided as arguments. On Windows, the implementation simply specifies the button state in both arguments (raw button state which is basically a bool and glut button state). I'm not familiar with the hid raw data format for the Logitech key states, so I can't really fix this myself. |
|
@stonexjr: Any comments on my previous comment? |
|
Also wondered, in freeglut/freeglut/src/fg_internal.h it is stated that the SpaceMotion, SpaceRotation and SpaceButton callbacks are only implemented on X11. With merging this that is no longer true, no? |
|
@rcmaniac25 : Sorry for the late reply. I just double checked the implementation on Windows and found out it does indicate which button was pressed or released. Specifically, the glutSpaceballButtonFunc() takes a callback function void (func*)(int b1, int b2), whose argument b1 indicates button id (1: left button, 2: right button) and argument b2 indicates pressed (1) and released (0). See attached print out. @dcnieho : I'm not sure if it works on OS X. Or at least we can say 'presently implemented only on UNIX/X11 and Windows' |
|
@dcnieho : BTW, shall we also update the API documentation for glutSpaceballXXX at http://freeglut.sourceforge.net/docs/api.php#WindowCallback ? |
|
@stonexjr : it would be great if you could update that yes! Thanks! |
|
@stonexjr: ah, I miss read the source. Yeah, it's fine then. |
|
@dcnieho : OK, I have created a new pull request based on the following updates: |
Added support of 3Dconnexion Space Navigator on Windows.
Note that the default macro _WIN32_WINNT defined in the visual studio project file (generated by CMake) is smaller than 0x603 which prevents the usage of WM_INPUT and RAMWINPUTDEVICE. An work around solution is simply remove the macro predefined in the visual studio Configuration Property | C/C++ | Preprocessor.