-
Notifications
You must be signed in to change notification settings - Fork 113
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
[RSDK-1705] add colormap for SLAM UI #2160
Changes from 3 commits
0ef1f8c
d8ca9e3
066a615
328a6da
7225f65
8808bc2
36a9145
4d621a8
5ad2518
4f7df84
bcefc48
a6d7e6f
daa884a
c206cae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,36 @@ import { MapControls } from 'three/examples/jsm/controls/OrbitControls'; | |
import { PCDLoader } from 'three/examples/jsm/loaders/PCDLoader'; | ||
import type { commonApi } from '@viamrobotics/sdk'; | ||
|
||
/* | ||
* // Leaving additional color map commented for if we want to change to a different scheme. | ||
* // const colorMapViridis = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mind adding the url that defined this color map? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [q] Why are we storing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left the viridis colormap commented out so we could easily swap to it if after testing we found the greyscale color map did not perform the way we wanted There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha so the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the viridis color map is a separate mapping from the greyscale color map There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @EshaMaharishi just confirming we are alright having this alternative color schema as a comment in this file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally I think it's better to remove unused code than leave it commented out, but I don't feel very strongly in this case if we think it's useful to have it available There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On second thought, I think we should remove it. It's not great to accumulate commented out code in a codebase. It'll always be in the git history if we need it. |
||
* // [253, 231, 37], | ||
* // [181, 222, 43], | ||
* // [110, 206, 88], | ||
* // [53, 183, 121], | ||
* // [31, 158, 137], | ||
* // [38, 130, 142], | ||
* // [49, 104, 142], | ||
* // [62, 73, 137], | ||
* // [72, 40, 120], | ||
* // [68,1,84] | ||
* // ] | ||
*/ | ||
|
||
// this color map is greyscale | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mind adding the url that defined this color map? |
||
const colorMapGrey = [ | ||
[247, 247, 247], | ||
[239, 239, 239], | ||
[223, 223, 223], | ||
[202, 202, 202], | ||
[168, 168, 168], | ||
[135, 135, 135], | ||
[109, 109, 109], | ||
[95, 95, 95], | ||
[74, 74, 74], | ||
[0, 0, 0], | ||
]; | ||
JohnN193 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const props = defineProps<{ | ||
name: string | ||
|
||
|
@@ -78,6 +108,21 @@ const disposeScene = () => { | |
scene.clear(); | ||
}; | ||
|
||
// Find the desired color bucket for a given probability. This assumes the probability will be a value from 0 to 100 | ||
const probToColorMapBucket = (normProb: number, numBuckets: number): number => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please add unit tests for this function that shows how it behaves with probabilities both within the expected range & outside the expected range and with varied numbers of buckets? You should be able to follow a similar pattern to what is done here: https://github.com/viamrobotics/rdk/blob/main/web/frontend/tests/unit/Base.spec.ts#L1 I'm happy to pair with you on it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No current unit testing framework is set up yet for RDK, recommend just moving forward without tests until we complete https://viam.atlassian.net/browse/APP-1600 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cool stuff, added a ticket for SLAM to make some tests after that one https://viam.atlassian.net/browse/RSDK-2606 |
||
const prob = Math.max(Math.min(100, normProb * 255), 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [q] Isn't the normProb an integer between 1-100, why do we need to multiply it by 255? I would expect this to be:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the PCD loader actually sets the values on a scale from 0 to 1, which is why I referred to it as a normalized probability( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm @JohnN193 , for my understanding is it really a scale from 0 to 1, or is it 0 to 100/255? Also, "normalized" seems to be its own concept in BufferAttribute so it'd be nice to avoid a name could be confused with that... could we simply call it 'probability'? [note to self] carto_grpc_server sends back a value from 0-100, then PCDLoader divides by 255. We multiply by 255 here to bring it back to 0-100. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah for our implementation of a PCD from cartographer, normProb goes from 0-100/255. Because that isn’t a range I can guarantee for all slam algos I added the max/min bounds. Fine with calling it probability. I picked the name just because I saw it was normalized but I don’t feel strongly about it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, calling it probability sounds good to me. Free tor resolve from my side. |
||
return Math.floor((numBuckets - 1) * prob / 100); | ||
}; | ||
|
||
/* | ||
* Map the color of a pixel to a color bucket value. | ||
* normProb is the probability value normalized by the size of a byte(255) to be between 0 to 1. | ||
*/ | ||
const colorBuckets = (normProb: number): THREE.Vector3 => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this also be unit tested? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added a ticket to unit test this https://viam.atlassian.net/browse/RSDK-2606 |
||
return colorMapGrey.map(([red, green, blue]) => | ||
new THREE.Vector3(red, green, blue).multiplyScalar(1 / 255))[probToColorMapBucket(normProb, colorMapGrey.length)]!; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this normalizes the pixels of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GPUs usually like data normalized to 0-1 :) |
||
}; | ||
JohnN193 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const updateCloud = (pointcloud: Uint8Array) => { | ||
disposeScene(); | ||
|
||
|
@@ -110,6 +155,15 @@ const updateCloud = (pointcloud: Uint8Array) => { | |
intersectionPlane.position.set(center.x, 0, center.z); | ||
raycaster.objects = [intersectionPlane]; | ||
|
||
const colors = points.geometry.attributes.color; | ||
EshaMaharishi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// if the PCD has a color attribute defined, convert those colors using the colorMap | ||
if (colors instanceof THREE.BufferAttribute || colors instanceof THREE.InterleavedBufferAttribute) { | ||
EshaMaharishi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for (let i = 0; i < colors.count; i += 1) { | ||
const colorMapPoint = colorBuckets(colors.getZ(i)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [q] Why do we get the probability number from Z? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We make the assumption in our current slam implementation that probability will live in the blue channel of the PCD, by setting the color integer to a value from 0 to 100. 100 is less than 255 so only the blue channel needs to be evaluated. The way the color attributes appear to work for three.js is the RGB fields correspond to xyz fields. I did not dive into three.js documentation to see how There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little confused b/c I thought that we put the probability data in the R channel here: https://github.com/viamrobotics/viam-cartographer/pull/25/files#diff-92ff8a5cd3a57430d78c6ba3b61a91d1810399b77613eece3c55841868d2822eR56 @jeremyrhyde @JohnN193 am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah yeah, int rgb = 0; I am not 100% certain how the pcdloader is handling the 4 byte integer, but experimentally this was found to be the Z channel(for cartographer PCDs the other two channels are 0) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome! Thanks for explaining!!! Would you mind please creating a ticket for the fact that currently the remote-control frontend & any users of slam need to have intimate knowledge of how the specific slam algo implementation encodes probability information the color integer value in the PCD (which currently only works for cartographer) so there is product awareness & we can prioritize when to come back to it to make it more generic / type safe / documented & add a comment at the top of this file with both the assumption this component makes about the PCD color information & a link to the ticket to come back to re assess those assumptions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for explaining! It sounds like since carto_grpc_server is returning a PCD with format "x y z rgb" and putting a probability value 0-100 in the 'rgb' field. Since 0-100 is guaranteed to be less than 255, the probability will end up in the lowest byte, so we're able to pull it out by looking at the 'b' channel. Agreed this is pretty roundabout. To consider in RSDK-2605: I wonder if we could use a PCD format "x y z intensity" and store the probability in intensity, since it looks like the PCDLoader would be able to pull it out as a single value: https://github.com/mrdoob/three.js/blob/0fbae6f682f6e13dd9eb8acde02e4f50c0b73935/examples/jsm/loaders/PCDLoader.js#L429. (In that case I think we'd need to use setXYZW below) |
||
colors.setXYZ(i, colorMapPoint.x, colorMapPoint.y, colorMapPoint.z); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @micheal-parks Just confirming: Is this the idiomatic way to set the color scheme of points in three.js? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @micheal-parks do you know why three.js uses setXYZ instead of something like setRGB for colors? Is it because the color type is a more generic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly, it's because the |
||
} | ||
} | ||
|
||
scene.add(points); | ||
scene.add(marker); | ||
scene.add(intersectionPlane); | ||
|
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.
[minor] expand on this comment to state that this schema is used to map probability values (probability value ranges?) to colors specified by the schema
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.
tweaked the comment to try being more reflective of what the usage is