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

Added maximumCachedBytes as a UPROPERTY #635

Merged
merged 4 commits into from
Sep 14, 2021
Merged

Added maximumCachedBytes as a UPROPERTY #635

merged 4 commits into from
Sep 14, 2021

Conversation

shikharvashistha
Copy link
Contributor

@shikharvashistha shikharvashistha commented Sep 7, 2021

Greetings of the day @kring,

Please review the PR and let me know in case of any changes if required.
Fixes : #633

  • Signed CLA

@cesium-concierge
Copy link

Thanks for the pull request @shikharvashistha!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

@kring
Copy link
Member

kring commented Sep 7, 2021

Hi @shikharvashistha, thanks for the PR! Don't worry about the CI failure (it currently doesn't work from external PRs). Before this can be said to fix #633, though, it needs a couple of things:

  1. The maxiumCachedBytes needs to be copied into the TilesetOptions instance. Take a look at how MaximumScreenSpaceError is handled.
  2. The default value and comments should be copy/pasted from TilesetOptions source code in cesium-native: https://github.com/CesiumGS/cesium-native/blob/a8f39d8cc6a45e2c5b845d4f596394e02b1e6cf7/Cesium3DTilesSelection/include/Cesium3DTilesSelection/TilesetOptions.h#L160

@shikharvashistha
Copy link
Contributor Author

shikharvashistha commented Sep 7, 2021

Hi @shikharvashistha, thanks for the PR! Don't worry about the CI failure (it currently doesn't work from external PRs). Before this can be said to fix #633, though, it needs a couple of things:

  1. The maxiumCachedBytes needs to be copied into the TilesetOptions instance. Take a look at how MaximumScreenSpaceError is handled.
  2. The default value and comments should be copy/pasted from TilesetOptions source code in cesium-native: https://github.com/CesiumGS/cesium-native/blob/a8f39d8cc6a45e2c5b845d4f596394e02b1e6cf7/Cesium3DTilesSelection/include/Cesium3DTilesSelection/TilesetOptions.h#L160

@kring Done !

@shikharvashistha shikharvashistha changed the title Added maxiumCachedBytes as a UPROPERTY Added maximumCachedBytes as a UPROPERTY Sep 7, 2021
@shikharvashistha
Copy link
Contributor Author

shikharvashistha commented Sep 7, 2021

Travis CI Logs

C:/Users/travis/build/CesiumGS/packages/CesiumForUnreal/HostProject/Plugins/CesiumForUnreal/Source/CesiumRuntime/Private/Cesium3DTileset.cpp(1220): error C2039: 'MaximumCachedBytes': is not a member of 'Cesium3DTilesSelection::TilesetOptions'
    C:\Users\travis\build\CesiumGS\packages\CesiumForUnreal\HostProject\Plugins\CesiumForUnreal\Source\ThirdParty\include\Cesium3DTilesSelection/TilesetOptions.h(56): note: see declaration of 'Cesium3DTilesSelection::TilesetOptions'
    C:/Users/travis/build/CesiumGS/packages/CesiumForUnreal/HostProject/Plugins/CesiumForUnreal/Source/CesiumRuntime/Private/Cesium3DTileset.cpp(1220): error C2039: 'maximumCachedBytes': is not a member of 'ACesium3DTileset'

@javagl
Copy link
Contributor

javagl commented Sep 7, 2021

The capitalization is reversed in this line. It is

options.MaximumCachedBytes = static_cast<int64_t>(this->maximumCachedBytes); but should be
options.maximumCachedBytes = static_cast<int64_t>(this->MaximumCachedBytes);

Also, the default value for the MaximumCachedBytes is currently 256, but should be 256 * 1024 * 1024 (256 Megabytes/mebibytes)

@shikharvashistha
Copy link
Contributor Author

The capitalization is reversed in this line. It is

options.MaximumCachedBytes = static_cast<int64_t>(this->maximumCachedBytes); but should be
options.maximumCachedBytes = static_cast<int64_t>(this->MaximumCachedBytes);

Also, the default value for the MaximumCachedBytes is currently 256, but should be 256 * 1024 * 1024 (256 Megabytes/mebibytes)

Fixed ! 👍

@kring
Copy link
Member

kring commented Sep 9, 2021

Just those two small things left @shikharvashistha and then we'll merge this. Thanks for sticking with it.

@kring
Copy link
Member

kring commented Sep 10, 2021

Looks like you've removed the options.maximumCachedBytes assignment entirely. You need that, it's only the cast you don't need.

@kring
Copy link
Member

kring commented Sep 14, 2021

Thanks @shikharvashistha!

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.

Expose maximumCachedBytes as UProperty
4 participants