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

[RSDK-1705] add colormap for SLAM UI #2160

Merged
merged 14 commits into from
Apr 10, 2023
2 changes: 1 addition & 1 deletion web/frontend/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@viamrobotics/remote-control",
"version": "0.2.10",
"version": "0.2.11",
"license": "Apache-2.0",
"type": "module",
"files": [
Expand Down
54 changes: 54 additions & 0 deletions web/frontend/src/components/slam-2d-render.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

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

Copy link
Member Author

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

* // const colorMapViridis = [
Copy link
Member

Choose a reason for hiding this comment

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

Mind adding the url that defined this color map?

Copy link
Contributor

Choose a reason for hiding this comment

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

[q] Why are we storing the colorMapViridis here if we are using the grayscale one?

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha so the colorMapViridis is given to that grayscaler to produce our grayscale map?

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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],
];

const props = defineProps<{
name: string

Expand Down Expand Up @@ -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 => {
Copy link
Member

@nicksanford nicksanford Apr 5, 2023

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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:

const prob = Math.max(Math.min(100, normProb), 0);

Copy link
Member Author

Choose a reason for hiding this comment

The 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(normProb for short)

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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 => {
Copy link
Member

Choose a reason for hiding this comment

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

Can this also be unit tested?

Copy link
Member Author

Choose a reason for hiding this comment

The 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)]!;
Copy link
Member

Choose a reason for hiding this comment

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

What does the .multiplyScalar(1 / 255) do?

Copy link
Member Author

Choose a reason for hiding this comment

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

this normalizes the pixels of colorMapGrey. So the vector3 will contain values of 0 to 1 instead of 0 to 255, which would match the expected values that colors wants

Copy link
Member

Choose a reason for hiding this comment

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

GPUs usually like data normalized to 0-1 :)

};

const updateCloud = (pointcloud: Uint8Array) => {
disposeScene();

Expand Down Expand Up @@ -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;
// if the PCD has a color attribute defined, convert those colors using the colorMap
if (colors instanceof THREE.BufferAttribute || colors instanceof THREE.InterleavedBufferAttribute) {
for (let i = 0; i < colors.count; i += 1) {
const colorMapPoint = colorBuckets(colors.getZ(i));
Copy link
Member

Choose a reason for hiding this comment

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

[q] Why do we get the probability number from Z?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 loader.parse explicitly handles this

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

@JohnN193 JohnN193 Apr 5, 2023

Choose a reason for hiding this comment

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

ah yeah, cairo has probability encoded in the R channel, but after we math out the probability value, pass that char into the integer field of the PCD. the integer field(calling it rgb for simplicity) works as follows

int rgb = 0;
rgb = rgb | (red << 16);
rgb = rgb | (green << 8);
rgb = rgb | (blue << 0);
viam::utils::writeIntToBufferInBytes(pcd_data, RGB);

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)

Copy link
Member

@nicksanford nicksanford Apr 5, 2023

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

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

Yep!

Copy link
Contributor

Choose a reason for hiding this comment

The 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 THREE.BufferAttribute

Copy link
Member

Choose a reason for hiding this comment

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

Exactly, it's because the BufferAttribute class is used for any all sorts of vector data, like colors, vertices, normals ,etc, and the authors haven't added a setRGB alias function

}
}

scene.add(points);
scene.add(marker);
scene.add(intersectionPlane);
Expand Down