AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.41k stars 289 forks source link

Output CPU type, (available) video memory in logfile.txt. #250

Closed hsnabn closed 7 years ago

hsnabn commented 8 years ago

It wouldn't hurt, would it?

I know that the processor count is already output, but CPU type could also be output (for example, Intel i7-4790K).

By video memory, I mean the GPU's dedicated memory (shared memory is not important, I think?). If possible, only the available amount could be output. (EDIT: Now that I think about it, total amount may also be useful?)

However, I understand these may be hard to implement, even if they sound simple. In that case, disregard this. The same information can be obtained in other ways (the person with a problem could manually provide it).

ilexp commented 8 years ago

More diagnostic info indeed wouldn't hurt. Some of it may be harder to obtain through .Net / OpenGL, but we'll see. :)

ilexp commented 8 years ago

Potentially useful example: How to Get Hardware Information in .Net. In any case, this should go into the (DotNet / Windows) backend and should be guarded with try / catch and considered very carefully - especially since any of this information might (?) require admin privileges or trigger security alerts.

It should also be checked whether this would also work on Mono. If it doesn't, determine if there is a Mono / DotNet way to do it. This is not a strict requirement, but until now, the DotNetFramework backend is potentially Mono-compatible as well and it might not be worth sacrificing this.

Safety first.

Edit: After testing the example project from the above link, I'm a bit less convinced to use this approach. It seems like a complete overkill - you can actually query pretty much everything with this - and 10% of the available hardware info queries result in the program freezing and me having to use the task manager to close it again.

Will need to look for another way. This seems like it's just "too much" for a simple diagnostic info log. If there is no other / simpler / safer / less intrusive way, consider closing this.

Xinayder commented 8 years ago

There's an environment variable on Windows that outputs information about the CPU:

%PROCESSOR_LEVEL%
%PROCESSOR_ARCHITECTURE%
%PROCESSOR_IDENTIFIER%
%PROCESSOR_REVISION%

And here's my output:

6
AMD64
Intel64 Family 6 Model 58 Stepping 9, GenuineIntel
3a09
hsnabn commented 8 years ago

Wouldn't RockyTV's method work for even non-Windows (Mono)? I mean, accessing the System.Environment class for information.

HamishBuckmaster commented 7 years ago

Hi, I'll look into adding this into the logfile and commit this. Can i get added to the contributors list?

ilexp commented 7 years ago

Hey there,

good to hear you're considering to contribute to this project!

Adding people to the contributors list is not the primary contribution path for this project. Instead, the usual workflow is to fork the project, implement your contribution in a feature / fix branch and then submit a Pull Request, which then gets reviewed and either merged, iterated on or closed. Please read the pull request checklist before proceeding.

Since this issue doesn't have an "obvious solution" it can be a good idea to outline the changes you are planning to do first, so we can discuss the approach you're taking and early-out if that's not what we think is a good fit for the existing framework.

SirePi commented 7 years ago

@HamishBuckmaster Since I already know this comment will come from @ilexp, I'll just save him the time 😛 Change the indentation spaces in your code with tabs, as it was in the source.

Nothing bad in using spaces, but tabs are the way in Duality

ilexp commented 7 years ago

@SirePi See the PR, already mentioned that in the review :) This is by far the most common mistake in PRs. Should definitely investigate Roslyn Analyzers and maybe make this a compile error to catch this sooner.

SirePi commented 7 years ago

Aaaand I'm dumb

Xinayder commented 7 years ago

@ilexp just add a .editorconfig in the project

ilexp commented 7 years ago

@RockyTV Didn't really get VS .editorconfig support to work so far - do you have a source on how to use that with VS, ideally globally to the entire solution?

Xinayder commented 7 years ago

@ilexp https://github.com/godarklight/DarkMultiPlayer/blob/master/.editorconfig

For VS2015, you need to install the extension that adds support for .editorconfig. If you use VS2017, it is bundled with the software.

ilexp commented 7 years ago

@RockyTV Great, thanks! Moving this into issue #537 so we don't spam this one any further.

SirePi commented 7 years ago

yoink!

ilexp commented 7 years ago

yoink!

Since I can't officially assign you via GitHub UI, consider yourself informally assigned 👍

SirePi commented 7 years ago

This brought to light another issue in the preexisting OS version determination code. Per MSDN

Starting with Windows 8, the OSVersion property returns the same major and minor version numbers for all Windows platforms. Therefore, we do not recommend that you retrieve the value of this property to determine the operating system version.

In fact, my Win 10 is detected as Win 8 (v 6.2).

Is any correction required at this stage?

ilexp commented 7 years ago

Is any correction required at this stage?

It's non-critical, but if you had a safe alternative way to retrieve that value that works better than the OSVersion, that would be worth a try. Otherwise keep it for now.

SirePi commented 7 years ago

Apparently the easiest way (no changes on code side, if I understood it correctly) would be to add an executable manifest file. https://msdn.microsoft.com/en-us/library/windows/desktop/dn481241%28v=vs.85%29.aspx

If this is not a viable solution, maybe at least remove specific indication of Windows version (7, 8, ...) with a generic "Windows" label?

ilexp commented 7 years ago

Apparently the easiest way (no changes on code side, if I understood it correctly) would be to add an executable manifest file.

Would be okay with that if it can be ruled out that stating OS compatibility via manifest file has any other relevant effects on .NET API or application behavior. However:

Therefore, we do not recommend that you retrieve the value of this property to determine the operating system version.

Doesn't sound like that property should be used at all anymore. Do they propose any alternatives that should be preferred?

If this is not a viable solution, maybe at least remove specific indication of Windows version (7, 8, ...) with a generic "Windows" label?

Sounds good for a first workaround. Maybe just change "Windows 8" to "Windows 8 or higher", so we can at least get info about earlier version differences.

ilexp commented 7 years ago

Done by @SirePi.