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

Tweaks to performance testing framework. #1200

Merged
merged 23 commits into from
Sep 25, 2023

Conversation

kring
Copy link
Member

@kring kring commented Aug 31, 2023

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.

@cesium-concierge
Copy link

Thanks for the pull request @kring!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

Comment on lines 84 to 85
"Cesium3DTiles",
"Cesium3DTilesReader",
Copy link
Member Author

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.

Comment on lines +895 to +897
if (!this->_tilesToHideNextFrame.empty()) {
this->LoadProgress = glm::min(this->LoadProgress, 99.9999f);
}
Copy link
Member Author

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();
Copy link
Member Author

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.

Comment on lines 301 to 303
if (afterTest) {
afterTest(context);
}
Copy link
Member Author

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.

Copy link
Contributor

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);
Copy link
Member Author

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?

Copy link
Contributor

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().

Copy link
Contributor

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);
Copy link
Member Author

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.

Copy link
Contributor

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

Copy link
Contributor

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:

  1. Executing tasks at the end of frame
    image

  2. A special tick call in the main tick loop
    image

I haven't hooked into those yet for these reasons

  1. I haven't got that far yet. The original test was made as quickly and simply as possible
  2. I like the control of determining how often the tick loop executes, what tick times are used, and what is getting ticked
  3. 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?

Copy link
Contributor

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...

Copy link
Contributor

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.

@kring
Copy link
Member Author

kring commented Aug 31, 2023

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
@csciguy8
Copy link
Contributor

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 LoadProgress was calculated as 100, but wasn't really done yet. Was testing Google Tiles, Unreal Insights capture below...

image

@csciguy8 csciguy8 marked this pull request as ready for review September 25, 2023 20:47
@csciguy8
Copy link
Contributor

csciguy8 commented Sep 25, 2023

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.

@csciguy8 csciguy8 merged commit bfac249 into simple-performance-test Sep 25, 2023
33 checks passed
@csciguy8 csciguy8 deleted the performance-test-tweaks branch September 25, 2023 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants