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.

Sign up for GitHub

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

Jump to bottom

Add i3s catalog item #7158

Merged
merged 25 commits into from
Jul 31, 2024
Merged

Add i3s catalog item #7158

merged 25 commits into from
Jul 31, 2024

Conversation

ljowen
Copy link
Contributor

@ljowen ljowen commented May 20, 2024 •
edited
Loading

What this PR does

Fixes #6416

Test me

How should reviewers test this?
Init source:
http:https://ci.terria.io/add-i3s-support/#clean&https://gist.githubusercontent.com/ljowen/445d4ba31ef521b36875ae7555b8a78b/raw/a821943f2e993ca8ae23fe739e71b8ce258edc1e/frankfurtI3S.json

Other I3S examples:

Known Issues

  • Highlighting breaks when changing LoD, for example select a feature, zoom / pan away the feature will no longer be highlighted. (Similarly if a feature is selected while the dataset is loading in): See example here in cesium sandcastle: link
  • Some features such as searching and clipping box are not yet implemented

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

@ljowen ljowen linked an issue May 20, 2024 that may be closed by this pull request
Add cesium native I3S support #6416
Closed
@ljowen ljowen changed the title RFC: Add i3s support WIP: Add i3s support May 20, 2024
@ljowen ljowen force-pushed the add-i3s-support branch 2 times, most recently from 6525652 to bb12991 Compare May 20, 2024 23:19
@CLAassistant
Copy link

CLAassistant commented May 20, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

na9da
na9da reviewed May 22, 2024
View reviewed changes
Copy link
Collaborator

@na9da na9da left a comment

Choose a reason for hiding this comment

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

@ljowen - looking great and good job figuring out the complexities of the model layer. I have just added a few suggestions.

lib/Core/getDataType.ts Outdated Show resolved Hide resolved
lib/Models/Catalog/CatalogItems/I3SCatalogItem.ts Outdated Show resolved Hide resolved
lib/Models/Catalog/CatalogItems/I3SCatalogItem.ts Outdated Show resolved Hide resolved
lib/Models/Catalog/CatalogItems/I3SCatalogItem.ts Outdated Show resolved Hide resolved
lib/Traits/TraitsClasses/I3SCatalogItemTraits.ts Show resolved Hide resolved
lib/ModelMixins/Cesium3dTilesMixin.ts Outdated Show resolved Hide resolved
@ljowen ljowen force-pushed the add-i3s-support branch 4 times, most recently from dc8249e to 21a5faf Compare June 3, 2024 05:21
@ljowen ljowen changed the title WIP: Add i3s support Add i3s catalog item Jul 1, 2024
ljowen and others added 14 commits July 1, 2024 14:07
@ljowen
Fix type errors new in node18
32a7b9c 
Apply api changes from "commander"
Ignore [css] prop for styled components, explicitly ignore other props on bare html elements
@sixlighthouses @ljowen
Wps default date (#7026)
28d2e80 
* refactor the DateTimeParameterEditor to be a functional component

* add the proptypes check to appease the tests

* format and minor code tidy

* update CHANGES.MD

* add the currentTime from timelineStack as the default date time in WPS params

* no need to set dateValue when declaring it with useState

* remove unnecessary call to updateParameters

* check parameter.value when the DateTimeEditor reloads

* use moment to correctly format the date/time

* missing import for WebFeatureServiceCatalogGroupTraits

* add CHANGES entry

* clean up the initial load of DateTimeParameterEditor.jsx

* make DateTimeParameterEditor a tsx, remove useState

* clean up CHANGES

* update CHANGES.md
@glughi @ljowen
Fix Timeline that doesn’t restart after it has been closed once
cea7c68
@ljowen
WIP: stash
30148de
@ljowen
Expose tileset as mappable item, zoomTo other controls now working
eacb807
@ljowen
Add imagery lighting trait
ea08b3b
@ljowen
Fix mobx warnings, extract createCesium3DTileStyle
63daf99
@ljowen
Move I3S to remote datatype, Fix import
488ca81
@ljowen
Create Cesium3DTileStyleMixin
508250f
@ljowen
Add unit tests for I3SCatalogItem
4a22731
@ljowen
Fix failing tests, add support for shadows and colour blend
ef5d6ea
@ljowen
Add feature picking, pick vector features now async
9cb95c9
@ljowen
Feature highlighting now working, extract default highlight colour to…
0b1a32e 
… Cesium3dTilesStyleMixin

Reload i3s if previously destroyed
@ljowen
Add test for I3S featurePicking
da9e528
@ljowen ljowen force-pushed the add-i3s-support branch 2 times, most recently from 6fcab20 to b21172a Compare July 1, 2024 06:28
@ljowen
Fix lint warnings
f47e16f 
- update CHANGES.md
ljowen
ljowen commented Jul 1, 2024
View reviewed changes
lib/Models/GlobeOrMap.ts
) {
feature._cesium3DTileFeature.color = originalColor;
try {
feature._cesium3DTileFeature.color = originalColor;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This throws an error as internally the features model reference becomes stale when LoD changes

na9da
na9da requested changes Jul 4, 2024
View reviewed changes
Copy link
Collaborator

@na9da na9da left a comment

Choose a reason for hiding this comment

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

Looks good @ljowen - i have left some comments, will do another pass to check the feature picking.

Just noting some things we should tackle maybe in a separate PR:

  • Splitter support (easyish)
  • Clipping box (might need some thinking/refactoring)

lib/ModelMixins/Cesium3dTilesStyleMixin.ts Show resolved Hide resolved
lib/ModelMixins/Cesium3dTilesStyleMixin.ts Show resolved Hide resolved
lib/Models/GlobeOrMap.ts Outdated Show resolved Hide resolved
@ljowen
Copy link
Contributor Author

ljowen commented Jul 10, 2024

Looks good @ljowen - i have left some comments, will do another pass to check the feature picking.

Just noting some things we should tackle maybe in a separate PR:

  • Splitter support (easyish)
  • Clipping box (might need some thinking/refactoring)

I've added splitter support, I think Clipping box might have to be a follow up

@ljowen ljowen requested a review from na9da July 15, 2024 00:31
na9da
na9da approved these changes Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@na9da na9da left a comment

Choose a reason for hiding this comment

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

@ljowen - Looks good to go.

On the feature selection issue, we have the same problem for 3d-tiles so might be good to create a separate issue.

The build error seems to be from prettier.

@ljowen ljowen merged commit f29650d into main Jul 31, 2024
9 checks passed
@ljowen ljowen deleted the add-i3s-support branch July 31, 2024 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@na9da na9da na9da approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add cesium native I3S support
5 participants
@ljowen @CLAassistant @na9da @sixlighthouses @glughi