Page MenuHomeWildfire Games

Removes active values from UserReport
Needs ReviewPublic

Authored by vladislavbelov on Aug 26 2018, 3:35 AM.

Details

Reviewers
elexis
Summary

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.

Test Plan
  1. Apply the patch and compile the game.
  2. Run the game.
  3. 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 BuildJenkins

Event Timeline

vladislavbelov created this revision.Aug 26 2018, 3:35 AM
Vulcan added a subscriber: Vulcan.Aug 26 2018, 3:37 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/724/display/redirect

vladislavbelov edited the summary of this revision. (Show Details)Aug 26 2018, 3:38 AM

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.
The screen resolution is more relevant than the window size before the application even initialized.
It gives me 1024*768, but that's not the real window size.
Since we have the screen resolution below, ok.

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

vladislavbelov added inline comments.Aug 26 2018, 2:12 PM
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.

elexis added inline comments.Aug 26 2018, 2:24 PM
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.
The comment itself above the number could be sufficient when keeping the number so close to the call IMO.
Or you could move the version to the top of the function. So every reader that starts reading at the top of the function notices the comment.
(Perhaps or perhaps not even something like { "hwreport", 11 }.)

source/ps/GameSetup/HWDetect.cpp
319

OS = different drivers, build = possibly a different version, a different set of keys.

322

We can deduce it from the CPU model or round it to few decimal places.

I can refactor it into the proposed format above if you agree it's better.

source/ps/GameSetup/HWDetect.cpp
322

deduce it from the CPU model

Which source are you going to use for that, is it worth the hassle?

Vulcan added a comment.Sep 2 2018, 8:39 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/728/display/redirect

vladislavbelov added inline comments.Oct 29 2018, 4:59 PM
source/ps/GameSetup/HWDetect.cpp
322

Which source are you going to use for that, is it worth the hassle?

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.

Stan added a subscriber: Stan.Oct 30 2018, 9:52 AM
Stan added inline comments.
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.