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

GetVersionEx Deprecated Warning #263

Closed
chaigler opened this issue Nov 5, 2015 · 18 comments
Closed

GetVersionEx Deprecated Warning #263

chaigler opened this issue Nov 5, 2015 · 18 comments
Labels

Comments

@chaigler
Copy link
Contributor

chaigler commented Nov 5, 2015

T2D calls GetVersionEx (or, rather, GetVersionExW) in a few places in an attempt to determine the Windows version and do various hacky things (like work around very old Voodoo & Windows 2000 bugs...).

Unfortunately this is wrong on two fronts. First, GetVersionEx has been deprecated since Windows 8.1 and will return the wrong version information depending on app compatibility settings, etc. More importantly, the workarounds used by the engine really shouldn't be necessary anymore, and seem like old left overs from the TGE codebase. In particular, the engine does things like disable DirectInput & delete the GL context on versions of Windows older than Win2k. Is anyone still releasing games that support Windows 98 and below?

In my opinion the correct way to go about this is to remove the calls to GetVersionEx altogether and do some cleanup on the winInput.cc and winOGLVideo.cc so the engine stops trying to support 20 year old operating systems. The other approach is to check for the Windows version the "correct" way, like so: http://stackoverflow.com/questions/22303824/warning-c4996-getversionexw-was-declared-deprecated

@yurembo
Copy link
Contributor

yurembo commented Nov 5, 2015

I believe you should work with Windows NT only, this way to know correct OS version you should check ntdll.dll (http://yazevsoft.blogspot.ru/2015/09/windows-c-yurembo-windows-7-8.html)

@greenfire27
Copy link
Contributor

Thanks for pointing this out. I'll try to make these changes when possible. I definitely don't see a reason to support Windows 98 and below. If you want to take this a step further you could offer a pull request with your suggested changes. Thanks!

@yurembo
Copy link
Contributor

yurembo commented Nov 5, 2015

ok, I'll commit this

@yurembo
Copy link
Contributor

yurembo commented Nov 5, 2015

I am working with GitHub for the first time. How can I upload changes? I'm using TortoiseGit. What branch should I use?

@yurembo
Copy link
Contributor

yurembo commented Nov 5, 2015

I made this ))

@greenfire27
Copy link
Contributor

I've never used TortoiseGit so I can't tell you exactly. After you've tested an committed your changes I can help you submit a pull request though with just the needed commit.

@yurembo
Copy link
Contributor

yurembo commented Nov 5, 2015

I upload 4 files to development branch, then download and test Torque 2D.
Everything is fine!

2015-11-05 21:40 GMT+05:00 Peter Robinson notifications@github.com:

I've never used TortoiseGit so I can't tell you exactly. After you've
tested an committed your changes I can help you submit a pull request
though with just the needed commit.


Reply to this email directly or view it on GitHub
#263 (comment)
.

@greenfire27
Copy link
Contributor

I looked at your commit. You created a script function to return the WindowsNT version, which seems fine, but it doesn't really address the problems that chaigler brought up. Plus I'm not really sure why someone would need this info in script. I'll leave it out of the main repo but thanks for trying. I always appreciate help.

@yurembo
Copy link
Contributor

yurembo commented Nov 5, 2015

chaigler siad that he wants to know a clean way to get a Windows NT version number. I gave this method whilh would work on every NT system even on NT 10. If this function return 0 then it executes on older OS than NT.
Developer can use this function in the C++ code as well as in the console you see.

@greenfire27
Copy link
Contributor

True, but he mostly want to remove calls to GetVersionEx and remove code that supports very old operating systems. He only wanted a function like this in the event that we still want to keep code to support old system, which we don't. I agree with him that references to GetVersionEx and the hacks that they allow can be safely removed at this point.

@chaigler
Copy link
Contributor Author

chaigler commented Nov 5, 2015

That's a nifty way of testing for OS version, yurembo. I believe the stackoverflow post I linked also includes that method (but it pulls the info from kernel32 or some other system dll).

At any rate, I've removed the calls to GetVersionEx on my end but I'm hesitant to submit a pull request because in doing so I also removed the profileSystem() function. profileSystem() is responsible for loading prefs profiles based on the user's hardware config. It seemed like another bit of leftover fluff to me because it looks for profiles based on the user's processor (AMD or Intel) and Windows version. Processor family and Windows version aren't exactly good metrics for determining the capabilities of the user's system.

@greenfire27
Copy link
Contributor

It sounds like we're making progress then! So you've got the code that corrects the issue but you're worried that you took it too far when you removed profileSystem()? Is it possible to add that function back in, perhaps using yurembo's code? Between the three of us we should be able to figure this out.

@chaigler
Copy link
Contributor Author

chaigler commented Nov 6, 2015

One issue with yurembo's method is it returns the Windows version number, rather than the normally recognized 'product string' (i.e., Windows 8 will be reported as Win6.2, Windows 7 returns Win6.1, etc.).

The only place this code would be used is in profileSystem() to select the correct prefs profile file. profileSystem() would be looking for pref files with names like AMDWin6.2CardProfiles.cs, or IntelWin5.0CardProfiles.cs. Even if a developer wanted to use the profile system to develop hardware-specific default settings, the naming scheme isn't very intuitive.

For what it's worth, the current code considers everything above Windows 2000 to -be- Win2K so it's not particularly useful either way.

@greenfire27
Copy link
Contributor

I'm learning a lot here. Give me a day or two to review your code and then I'll most likely ask for a pull request.

@chaigler
Copy link
Contributor Author

chaigler commented Nov 6, 2015

Not a problem.

I've pushed the changes to winOGLVideo & winInput to the winogl_cleanup branch in my repo for anyone that's interested. It does away with the prefs profile system (on Windows, didn't mess with the other platforms) and removes the calls to GetVersionEx. No issues so far after some basic testing.

@greenfire27
Copy link
Contributor

Just looked at your repo. I like those branch names! Ogg support? Remove Unicows? Yes please!

By the way, have you contacted Mich about the steering committee yet?

@chaigler
Copy link
Contributor Author

chaigler commented Nov 7, 2015

Oops, meant to follow up on this. I contacted Mitch on Friday.

@greenfire27
Copy link
Contributor

I just finished looking through your changes. Everything looks good. I see no reason not to create a pull request. The profile system is unneeded, which is evident by that fact that all modern windows systems are treated the same and nobody has complained.

Make the pull request. Let's merge these changes in and close this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants