Currently we have active values in the UserReport, values that can be changed every second. They should go in a separate statistics group, but not in the hwdetect one.
Details
- Apply the patch and compile the game.
- Run the game.
- Check that reports work and there is no problem with hwdetect.js.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 6350 Build 10533: Vulcan Build Jenkins
Event Timeline
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/724/display/redirect
I looked at all values in the logfile from rP21868 and didn't see any dynamic values other than the ones removed here. So patch seems complete.
Also woudln't it be nicer to store the values as
{ "build": { .... }, "os": { .... }, "gl": { .... }, "cpu": { .... } }
?
Also for the backend, you might want to consider that possibly many will continue to use v11.
Also I broke the build, fixing...
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
276 | Agree that these can be nuked. | |
319 | Arugably removable (even though it gives a possibly valuable indication even if only measured at gamestart) | |
322 | Not sure about this one, it gives me "cpu_frequency": -1, "x86_frequency": 3392695955.358982, So it gives an indication on the performance of the users CPU, even if inaccurate, no? | |
340 | should be moved to the top of the file, a const, like #define PS_PROTOCOL_VERSION 0x01010015 // Arbitrary protocol |
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
319 | But it's still active and it's not about hwdetect. My understanding of hwdetect, that the report data should change if and only if hardwares or drivers for hardwares were changed. | |
322 | It's not a static value, we measure it at runtime, so we have a visible error here. | |
340 | Why on top? I placed it here, because you may see it after making all changes, but on top it's easier to forget, no? I'd prefer static cost int for on top to fix the type. |
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
319 | (or OS or build or those becoming separate reports too) | |
322 | So we have a somewhat inaccurate number instead of no number. Or can we deduce the frequency from the other data? (I doubt) | |
340 | I guess it's ok to keep it inside this function as long as this function is the only one relevant to the hwreport and the version only refering to the format of the hwreport part of the UserReport. |
I can refactor it into the proposed format above if you agree it's better.
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
322 |
Which source are you going to use for that, is it worth the hassle? |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/728/display/redirect
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
322 |
Vendor technical reports. The current value is useless for us, because it can be any, it depends on a current machine loading. So I suggest to remove this value, until we have a better one. |
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
322 | Won't the user report be false if the person overclocked his CPU ? If we have both values we can compare. |
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
322 | I'd say semi-false, because we'll know a value less than the actual value. The current value is pretty dynamic and has noticeable error (we should round it at least). Also we calculate it in the pregame, so the value will be less than possible. |
Just tested the patch, compiled enabled feedback, didn't get any errors.
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
276 | What about people that play in windowed mode ? Don't we want know in what resolution they play ? | |
319 | Agree that it is irrelevant especially since we are still a 32bit process and knowing that 16GB are free on 20 doesn't seem like a really useful information. | |
322 | Not sure about removing it but the points seem good enough to remove it. | |
340 | Wouldn't it be nicer to have it in the default.cfg file ? to update it at each release ? This way if someone forgets you don't have to rebuild the whole game. |
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
340 | Well you have mod.cfg, local.cfg, and user.cfg that should do the trick :) → The version is independent of the release cycles. True. However you might want to separate the hardware by the version people play, so you can make stats per version. Like it's more useful to know the hardware used in the last 4 versions than in the last 20. |
I guess you were waiting for the green color.
One could do a completeness test and see if more stuff could be removed, otherwise there is nothing else to confirm that I see.
Btw I wonder if one couldn't use some C++ magic to avoid the hundreds of copies of scriptInterface.SetProperty and use something similar to init lists { std::make_pair("property", "value"), ... }
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
319 | The purpose of the data processing is to see for what capabilities we can build. So it's (as) valuable to know whether people nowadays have more than 4GB ram. One could argue that if the available memory is useful to be measured, it should not be deleted but moved to a new report. I don't see anyone caring about that number, so... |
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
340 | But config settings are used to change the program flow, this is a piece of information that if changed by a user, would indicate something incorrect, no? So more comparable to the protocol version magic of the netclient, or the pyrogenesis engine version constant. The user can't change anything in JS or the GUI that would influence the version. |
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
340 | Fair point. I just imagined a random case where someone changed the feedback link for his server and wanted to start at 0 for his own mod. |
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
340 | He then either does not change this protocol and thus has no reason to use a different protocol version number, |