-
Notifications
You must be signed in to change notification settings - Fork 295
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
Tweaks to performance testing framework. #1200
Conversation
Thanks for the pull request @kring!
Reviewers, don't forget to make sure that:
|
"Cesium3DTiles", | ||
"Cesium3DTilesReader", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed after the cesium-native upgrade that fixes CesiumGS/cesium-native#717.
if (!this->_tilesToHideNextFrame.empty()) { | ||
this->LoadProgress = glm::min(this->LoadProgress, 99.9999f); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because tile hides are always delayed by one frame, it was possible before for the performance test to end (and suspend further updates) with tiles of multiple LODs visible simultaneously. This change makes it so that loading isn't 100% complete until these tiles have been hidden.
@@ -2064,6 +2069,8 @@ void ACesium3DTileset::Tick(float DeltaTime) { | |||
updateTileFade(pTile, false); | |||
} | |||
} | |||
|
|||
this->UpdateLoadStatus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be moved later, after _tilesToHideNextFrame
has been set.
if (afterTest) { | ||
afterTest(context); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This afterTest
thing allows doing additional checks after the performance test is complete. For example, to make sure the test did what you expected. Or in my case, to do an additional test after zooming out.
In retrospect, though, I think maybe it would be better to restructure things further so that these std::functions aren't needed at all. For example, the Denver test could instead look like this:
LoadTestContext context;
createCommonWorldObjects(context);
setupForDenver(context);
RunLoadTest(contex);
Where RunLoadTest
starts with "Halt tileset updates and reset them", skipping the context and object creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. The RunLoadTest
part is really the test code we care about. The setupForXXX
should be clearly separate and reusable. What if we want this setup code for other tests?
zoomedOut.FieldOfViewDegrees = 90; | ||
context.setCamera(zoomedOut); | ||
|
||
context.pawn->SetActorLocation(zoomedOut.Location); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird that we need to setCamera
and also set the DynamicPawn's location. Why do we need the custom camera at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, we don't need it. Setting the pawn's location / rotation should be enough to run the test.
This was leftover setup code when I was playing with creating a world outside of the editor. It was needed this because ACesium3DTileset
wasn't picking up any cameras in ::GetCameras()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we do need it now. Just committed changes to start / end PIE mode, and in this mode, it does use this additional camera rather than the active viewport camera.
} | ||
}); | ||
|
||
TestEqual("visibleTiles", visibleTiles, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This unfortunately fails, and it calls attention to a bit of a problem in the performance testing framework. The problem is that the performance test is executed in an Editor world, which means that tiles are selected using any active editor viewports. That's a bit problem because different editor views will affect the performance, and in this particular case it also causes my test to fail because the editor viewport is zoomed in close and causes additional tiles to be selected. To see what I mean, switch your default Editor viewport to an orthographic view (click Perspective, then Top) and note that the test now passes.
In general, I worry that the way the performance test is executed here is just a little bit artificial, and that will affect our results in unknown ways. I think it would be better to use Play-in-Editor mode and let the Engine itself manage the ticking, by following the model for this that's used in the sublevel tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I never really liked how it just used an active Editor viewport. This needs to be more rigid and predictable.
Committed some changes to start / end PIE mode, which simultaneously
- Uses the additional camera (and viewport) that each location configures
- Fixes your test so that it passes
- Is less artificial. Calls typical in-game functions like ::BeginPlay ::BeginDestroy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to use Play-in-Editor mode and let the Engine itself manage the ticking, by following the model for this that's used in the sublevel tests.
Looks like the sublevel test uses two mechanisms for hooking into the game loop:
I haven't hooked into those yet for these reasons
- I haven't got that far yet. The original test was made as quickly and simply as possible
- I like the control of determining how often the tick loop executes, what tick times are used, and what is getting ticked
- The inevitable next steps are to set up repeatable benchmarks and profile problems. If the way we tick doesn't impact either of these, do we need to change it?
But, it might be the case in the future that we MUST hook into the main tick loop for one reason for another. For example, what if we discover we need a test to render to a texture, but nothing is showing up because of this hacky tick logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still thinking on this btw. Stay tuned...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, committed some changes that do a more 'normal' tick loop, using latent commands. Kind of a blend of what Unreal recommends here, and the test helper functions you use for the sublevels test.
Some of the comments above are suggestions for improvements, and some are commentary on the original PR. Sorry if that's a bit confusing, let me know if you have any questions @csciguy8. |
When PIE ends, it seems to clear all tiles. Before we froze the state of what the test loaded, which was useful. But it's not all that useful to just see nothing when the test ends. Let it stream it all back in. Also, move play session param to context so it can be configured if needed
Melbourne 'after' test is temporarily commented out
…cold / warm testing
Seems to yield more consistent test times
Helps see cases where work is still being done passed the end mark (which might be a bug)
This silences our ACesium3DTileset warning about this during tests.
Caught an interesting bug today. The test seems to have completed, but there was obviously more loading happening after the end mark. This means the reported time is faster than it actually is. This suggests that |
Merging this into the base. It doesn't make sense to review both as separate. Performance tests are a new effort and this branch is the better one. |
This is a PR into #1198 with some changes I made while debugging CesiumGS/cesium-native#717. I don't think you'll want to merge it as-is, which is why I've marked it draft. More for your consideration @csciguy8.
I'll add notes inline explaining these changes.