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

Accessor cache using CeslumAsync::SqliteCache #666

Merged
merged 15 commits into from
Feb 23, 2024
Merged

Conversation

timoore
Copy link
Contributor

@timoore timoore commented Feb 5, 2024

Addresses #650.

This builds on #634, which needs to be merged first.

@r-veenstra
Copy link
Contributor

r-veenstra commented Feb 6, 2024

I did some benchmarking with this USDA
load_test.zip

results:

First load, clear cache: 74 seconds, 116mb downloaded
Second load, cached: 44 seconds, 0mb downloaded
Restart app, load cached: 44 seconds, 0mb downloaded

Appears to be working as expected.

I do notice a rather large hang (~10-20 seconds) when I hit Reload all Tilesets in Cesium Debugging UI (this is what I'm doing to trigger a reload). This isn't unique to this branch, and I suspect it has something to do with unloading all of the tileset resources before it begins reloaded.

Point is, if we ignore the hang, the actual loading of the cached data goes down to ~24 seconds.

@r-veenstra
Copy link
Contributor

Experimented here with the clear cache functionality, it appears to be working well!

See the results below - times with the red dash were after clearing cache, the rest are a reload from cached data. Looking quite consistent.

image

@r-veenstra
Copy link
Contributor

Similar tests with Google Photorealistic 3D tiles

image

@timoore timoore changed the base branch from main to libcurl-accessor February 9, 2024 16:47
Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caching functionality is working great, I just have some comments about cache location and settings.

src/core/src/SettingsWrapper.cpp Show resolved Hide resolved
src/public/CesiumOmniverse.cpp Show resolved Hide resolved
src/core/src/Context.cpp Outdated Show resolved Hide resolved
src/core/src/Context.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The powertools extension is for our internal use and isn't included in the release zip, so if we expect users to be able to modify the cache settings it needs to move into the main cesium.omniverse extension. @r-veenstra would you have a recommended spot there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@r-veenstra r-veenstra Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timoore @lilleyse I'm thinking a small settings button could live here:
image

To get what i have in my image, replace line 191 in main_window.py with below

            with ui.HStack(height=24, spacing=3):
                self._profile_widget = CesiumOmniverseProfileWidget(self._cesium_omniverse_interface, height=20)
                self._add_button = ui.Button(
                    "",
                    image_url=f"{self._icon_path}/FontAwesome/plus-solid.png",
                    style=CesiumOmniverseUiStyles.setting_button_style,
                    enabled=True,
                    width=24
                )

And add this to styles.py

    setting_button_style = {
        "Button": {"padding": 0.0, "stack_direction": Direction.TOP_TO_BOTTOM, "margin": 0},
        "Button.Image": {
            "alignment": Alignment.CENTER,
        },
        "Button.Label": {"alignment": Alignment.CENTER_BOTTOM},
        "Button.Image:disabled": {"color": cl("#808080")},
        "Button.Label:disabled": {"color": cl("#808080")},
    }

The FA gear icon would be ok to use I think https://fontawesome.com/icons/gear?f=classic&s=solid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no problem with adding a settings button up there and having the cache parameters window evolve into a settings window. But are settings any less important than "Token", "Learn", and "Help"? What do you think about moving "Token" to the same settings window as the cache, and / or combining Learn, Help, and Settings into an "other" menu?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timoore I think we'd want to keep Token settings separate, as the are configured and stored per USD. I think we want to keep persistent extension settings separate.

It also introduces UI inconsistency across our runtimes, where the goal has been to keep the UI similar across Unity / Unreal / Omniverse.

You know, I just had another (and probably easier) thought. Let's just add another entry here:

image

Something like Cesium Settings or Cesium Extension Settings.

That way we don't need to touch any existing UI.

src/core/src/Context.cpp Show resolved Hide resolved
src/core/src/Context.cpp Outdated Show resolved Hide resolved
src/core/src/Context.cpp Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor

@timoore could you update CHANGES.md?

FilesystemUtil has functions for getting the user's home directory and
the preferred directory for a cache. Change Context setup code to use
these new functions.
src/core/include/cesium/omniverse/FilesystemUtil.h Outdated Show resolved Hide resolved
src/core/src/Context.cpp Show resolved Hide resolved
src/core/src/FilesystemUtil.cpp Outdated Show resolved Hide resolved
Base automatically changed from libcurl-accessor to main February 20, 2024 16:55
@lilleyse
Copy link
Contributor

I added this to CHANGES.md in #696. So no need to update the change log in this PR.

@timoore timoore marked this pull request as ready for review February 23, 2024 15:47
Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @timoore

@lilleyse lilleyse merged commit a7ec5d2 into main Feb 23, 2024
3 checks passed
@lilleyse lilleyse deleted the caching-accessor branch February 23, 2024 18:46
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.

4 participants