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

Win32: Default to XInput9_1_0.dll #3248

Closed
wants to merge 2 commits into from

Conversation

CookiePLMonster
Copy link
Contributor

This PR concludes the discussion from #2716.

PR consists of two separable commits, however they are logically related:

  • The first commit makes all Win32 examples link to xinput9_1_0.dll by default instead of xinput1_4.dll. This is realized by linking against xinput9_1_0.lib and making that compilation unit target Windows 7, unless specified otherwise.
  • The second commit addresses a nasty catch I spotted while working on the first one. When compiling examples, I noticed that XInput headers referenced XInput 1.4, but samples got linked against XInput 1.3. This could lead to some nasty bugs where the sample looks like it links against XInput 1.4, but in reality it doesn't. To address this, I verified all uses of DXSDK_DIR, and turns out they are all useless with the current setup. To avoid such ambiguities, I removed them all.

@ocornut
Copy link
Owner

ocornut commented May 18, 2020 via email

@CookiePLMonster
Copy link
Contributor Author

First of all, to provide context to used toolsets and IDEs - I have every Visual Studio from 2008 through 2019 installed on my PC. Therefore, what I see "by default" when opening project files on a clean repository is exactly what they are committed as, without any IDE upgrades.

  • Which platform SDK version and MSVC versions have tried this with?

imgui_examples.sln is a VS2015 solution with projects using a v110 (VS2012) toolset. Since this toolset predates the concept of selectable Windows SDK's, it uses a 8.0 SDK for me.

  • What happens when other compilation units target other Windows version or Xinput versions?

AFAIK nothing. Multiple XInput versions can be used at once without issues, and it should work fine by default. If somebody needs it to be the other way, they'll have to tailor those examples for their liking but that has to be done at the moment too, so nothing changes.

  • Ditto for linking, you state than linking 1.3 and including with 1.4 is problematic, how does using 9_1_0 with different variations of user applications using and linking other versions?

It's not problematic in the technical sense, but it is problematic in the sense that if you peek at headers, you expect 1.4 to be linked but then the inclusion of DXSDK_DIR in lib paths (but not include paths) links it against 1.3. This might lead to issues like "but hey, example code linked against 1.4 and it works on Windows 7 but my own application doesn't!" which the user will be scratching their head over unless they peek at the binary with Dependency Walker.

  • Afaik DXSDK_DIR is useful for pre 8.0 windows sdk.

Of course, but the way I see it, project configuration shouldn't be as generic as possible because people integrating imgui in their projects will not use it either way. This is merely a fix to make examples themselves consistent, and since they all target the 8.0 SDK by default, I consider it the target. If users need to change it, they'll need to modify the project either way and so they may as well end up re-adding DXSDK_DIR.

That said, you technically can still mismatch XInput 1.3 and 1.4, because the way they are picked differs greatly between the Windows SDK XInput and the DXSDK one. For Windows SDK, it is just:

#if(_WIN32_WINNT >= _WIN32_WINNT_WIN8)
#define XINPUT_DLL_A  "xinput1_4.dll"
#define XINPUT_DLL_W L"xinput1_4.dll"
#else 
#define XINPUT_DLL_A  "xinput9_1_0.dll"
#define XINPUT_DLL_W L"xinput9_1_0.dll"
#endif

meanwhile for DXSDK, it is

#ifndef XINPUT_USE_9_1_0
#define XINPUT_DLL_A  "xinput1_3.dll"
#define XINPUT_DLL_W L"xinput1_3.dll"
#else
#define XINPUT_DLL_A  "xinput9_1_0.dll"
#define XINPUT_DLL_W L"xinput9_1_0.dll"
#endif

I honestly don't know how to solve this incosistency without using dynamic linking. If the XInput library was loaded by LoadLibrary instead of using #pragma comment, imgui's code could just do LoadLibrary(XINPUT_DLL) and roll with it without any care for whether it's XInput9_1_0, XInput1_3 or XInput1_4.

@mirh
Copy link

mirh commented May 20, 2020

LoadLibrary doesn't sound bad either, but given that we already kind of assumed 9.1.0 is already more than enough, why can't you just define XINPUT_USE_9_1_0 and call it a day?

@CookiePLMonster
Copy link
Contributor Author

why can't you just define XINPUT_USE_9_1_0 and call it a day?

That's a valid point. The combination of targeting Win7 and XINPUT_USE_9_1_0 should ensure that by default only xinput9_1_0.dll is linked, no matter what SDK is used.

Updated comments to reflect an updated situation
with XInput libs.
@CookiePLMonster
Copy link
Contributor Author

Updated the PR with the suggestion from @mirh, because it solves the ambiguity of linked XInput libraries completely:

  • If the example uses XInput headers from Windows SDK, it picks xinput9_1_0 because of WINVER targetting Win7.
  • If the example uses XInput headers from DXSDK, it picks xinput9_1_0 because of XINPUT_USE_9_1_0 macro.

This should allow people to use this sample code in their own projects out of the box without any nasty surprises caused by their project settings. Of course, we only care about the default behaviour here, so it anyone wants to link against xinput1_3 or xinput1_4 instead, they need to edit the code - but that is not surprising, I think.

@ocornut
Copy link
Owner

ocornut commented Jan 25, 2021

Closing this as superseded and fixed by #3646

@ocornut ocornut closed this Jan 25, 2021
@CookiePLMonster CookiePLMonster deleted the xinput9_1_0 branch January 25, 2021 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants