-
Notifications
You must be signed in to change notification settings - Fork 44
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
chore(web): add multi feature selection APIs #716
Conversation
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #716 +/- ##
========================================
Coverage 26.73% 26.74%
========================================
Files 1571 1574 +3
Lines 171278 172027 +749
Branches 3896 3913 +17
========================================
+ Hits 45794 46004 +210
- Misses 124395 124934 +539
Partials 1089 1089
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Thanks for you hard work, I've added a few refactors. Rest is looking good to me.
@@ -747,8 +777,74 @@ function useSelection({ | |||
[], | |||
); | |||
|
|||
const selectedFeatureIds = useRef<{ layerId: string; featureIds: string[] }[]>([]); | |||
const selectFeatures = useCallback( |
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.
Can you divide the selectFeatures
function into smaller functions?
); | ||
else if (options) setSelectedLayer(s => [s[0], options, info]); | ||
else | ||
setSelectedLayer(s => (!s[0] && !s[1] && !s[2] ? s : [undefined, undefined, undefined])); |
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.
setSelectedLayer(s => (!s[0] && !s[1] && !s[2] ? s : [undefined, undefined, undefined])); | |
const resetSelectedLayer = (s) => (!s[0] && !s[1] && !s[2] ? s : [undefined, undefined, undefined]); | |
// Usage | |
setSelectedLayer(resetSelectedLayer); | |
colorScratch.green = Color.byteToFloat(pixels[index + 1]); | ||
colorScratch.blue = Color.byteToFloat(pixels[index + 2]); | ||
colorScratch.alpha = Color.byteToFloat(pixels[index + 3]); | ||
const object = context.getObjectByPickColor(colorScratch); |
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.
Can we wrap this in try and catch..?
return offCenter.computeCullingVolume(camera.positionWC, camera.directionWC, camera.upWC); | ||
} | ||
|
||
function getIntersection( |
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.
Its better to minimize side effects in functional programming. here It's modifying result directly. Instead, consider returning a new object:
function getIntersection(
a: BoundingRectangle,
b: BoundingRectangle,
result = new BoundingRectangle(),
): BoundingRectangle | undefined {
//...
return {
x: x1,
y: y1,
width: x2 - x1,
height: y2 - y1,
};
Just as a practice if you can apply similar thing to other functions that'll be great 🙏
const width = screenSpaceRectangle.width ?? 1; | ||
const height = screenSpaceRectangle.height ?? 1; | ||
const context = this._context; | ||
const pixels = context.readPixels({ |
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.
Now this one is optional,
You might want to add validations for the returned pixels
and handle potential issues gracefully, to avoid unexpected behavior in subsequent lines of code.
windowHeight: number, | ||
// TODO: Get condition as expression for plugin | ||
condition?: (f: PickedFeature) => boolean, | ||
) => PickedFeature[] | undefined; |
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.
consider returning an empty array as a falsy value instead of undefined
const viewer = cesium.current?.cesiumElement; | ||
if (!viewer || viewer.isDestroyed()) return; | ||
findFeaturesFromLayer(viewer, layerId, featureId, entity => { | ||
const tag = getTag(entity) ?? {}; |
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.
const tag = getTag(entity) ?? {}; | |
const tag = getTag(entity) ?? {}; | |
const updatedTag = { ...tag, isFeatureSelected: true }; |
Lets avoid direct manipulation
return findFeaturesFromLayer(viewer, layerId, featureIds, e => { | ||
return convertObjToComputedFeature(viewer, e)?.[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.
return findFeaturesFromLayer(viewer, layerId, featureIds, e => { | |
return convertObjToComputedFeature(viewer, e)?.[1]; | |
}); | |
return findFeaturesFromLayer(viewer, layerId, featureIds, e => convertObjToComputedFeature(viewer, e)?.[1]); |
Since you’re using arrow functions and implicit returns, consider adopting a more functional style for simple transformations. I might be worth doing the same for the rest of places where this practice can be adopted.
layerId: string, | ||
featureId: string[], | ||
convert?: (e: Entity | Cesium3DTileset | InternalCesium3DTileFeature) => T, | ||
): NonNullable<T>[] | undefined { |
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.
Instead of returning undefined, consider returning an empty array to keep the return
Same as above.[OPTIONAL]
@@ -151,6 +189,63 @@ export function findEntity( | |||
return; | |||
} | |||
|
|||
export function findFeaturesFromLayer<T = Entity | Cesium3DTileset | InternalCesium3DTileFeature>( |
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.
If possible, It'd be great if you can breakdown this function into smaller[OPTIONAL].
function filterEntitiesById(entities: Entity[], layerId: string, featureId: string[]): Entity[] {
// filtering logic
}
function getEntitiesFromDatasources(viewer: CesiumViewer, layerId: string, featureId: string[]): Entity[] {
// logic to get entities from datasources
}
function get3DTileFeatures(viewer: CesiumViewer, featureId: string[]): Set<Entity | Cesium3DTileset | InternalCesium3DTileFeature> {
// logic to get entities from 3D Tiles
}
Overview
I added some plugin API for multi feature selection.
What I've done
What I haven't done
How I tested
selectedFeatureColor
.selectedFeatureColor
.Which point I want you to review particularly
Memo