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

fix(pcd): sizeSum calculation + rgb parsing #3101

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(pcd): sizeSum calculation + rgb parsing #3101

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7a7003a
fix: sizeSum calculation
jclaessens97 Sep 27, 2024
0703ec4
fix: rgb parsing
jclaessens97 Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 11 additions & 14 deletions modules/pcd/src/lib/parse-pcd.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,20 +240,17 @@ function parsePCDHeader(data: string): PCDHeader {
pcdHeader.offset = {};

let sizeSum = 0;
if (pcdHeader.fields !== null && pcdHeader.size !== null) {
for (let i = 0; i < pcdHeader.fields.length; i++) {
if (pcdHeader.data === 'ascii') {
pcdHeader.offset[pcdHeader.fields[i]] = i;
} else {
pcdHeader.offset[pcdHeader.fields[i]] = sizeSum;
sizeSum += pcdHeader.size[i];
}
for (let i = 0, l = pcdHeader.fields.length; i < l; i++) {
if (pcdHeader.data === 'ascii') {
pcdHeader.offset[pcdHeader.fields[i]] = i;
} else {
pcdHeader.offset[pcdHeader.fields[i]] = sizeSum;
sizeSum += pcdHeader.size[i] * pcdHeader.count[i];
}
}

// for binary only
pcdHeader.rowSize = sizeSum;

return pcdHeader;
}

Expand Down Expand Up @@ -336,9 +333,9 @@ function parsePCDBinary(pcdHeader: PCDHeader, data: ArrayBufferLike): HeaderAttr
}

if (offset.rgb !== undefined) {
color.push(dataview.getUint8(row + offset.rgb + 0));
color.push(dataview.getUint8(row + offset.rgb + 1));
color.push(dataview.getUint8(row + offset.rgb + 2));
color.push(dataview.getUint8(row + offset.rgb + 2) / 255.0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree that using normalized colors is better. However this is a breaking change and should probably be mentioned in docs/upgrade-guide.md.

I am on the fence if it is worth it, but we could avoid breaking if we offered a pcd.normalizeColors loader option on the PCDLoader.

I have some concerns that we have code that creates a typed array from this array and if that typed array is Uint8Array it might fail (all values will be 0 or 1). Not sure if it is easy for you to find, I will try to dig more into this when I find some time.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, I'm perfectly fine by adding a normalizedColors option to avoid the breaking change!

Not sure what you mean by the last section about the Uint8Array. I think this is not applicable when we introduce a new array, right?

About the linting, I think I already ran the linter before pushing, but I can check again.

I wasn't able to run the tests so far. I had 2 failing thests from scratch, something to do with the paths I think, so if you have any pointers here, I'm happy to add some more tests with smaller examples!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what you mean by the last section about the Uint8Array. I think this is not applicable when we introduce a new array, right?

There is code in loaders.gl that can convert parsed data to e.g. binary representations, in which case it might copy this JS array into a Uint8Array.

I'd have to dig into exactly what is implemented for meshes in terms of conversion right now to verify if it is an issue at this time.

color.push(dataview.getUint8(row + offset.rgb + 1) / 255.0);
color.push(dataview.getUint8(row + offset.rgb + 0) / 255.0);
}

if (offset.normal_x !== undefined) {
Expand Down Expand Up @@ -400,13 +397,13 @@ function parsePCDBinaryCompressed(pcdHeader: PCDHeader, data: ArrayBufferLike):

if (offset.rgb !== undefined) {
color.push(
dataview.getUint8(pcdHeader.points * offset.rgb + pcdHeader.size[3] * i + 0) / 255.0
dataview.getUint8(pcdHeader.points * offset.rgb + pcdHeader.size[3] * i + 2) / 255.0
);
color.push(
dataview.getUint8(pcdHeader.points * offset.rgb + pcdHeader.size[3] * i + 1) / 255.0
);
color.push(
dataview.getUint8(pcdHeader.points * offset.rgb + pcdHeader.size[3] * i + 2) / 255.0
dataview.getUint8(pcdHeader.points * offset.rgb + pcdHeader.size[3] * i + 0) / 255.0
);
}

Expand Down
Loading