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

add calculate tangents #1389

Closed
wants to merge 2 commits into from
Closed

add calculate tangents #1389

wants to merge 2 commits into from

Conversation

xintongxia
Copy link
Contributor

@xintongxia xintongxia commented Jun 20, 2020

For #1387

Background

Change List

  • add calculate tangents and bitangents
  • add test app

TODO

  • add unit test
  • add doc
  • add an option for geometry class to support tangents and bitangents

Screen Shot 2020-06-19 at 5 10 08 PM

Copy link
Collaborator

@ibgreen ibgreen 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!

@@ -20,3 +20,4 @@ export {KeyFrames} from './animation/key-frames';

// Utils
export {default as ClipSpace} from './utils/clip-space';
export {calculateTangents} from './utils/tangent';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use and underscore export until we do API audit?

@@ -0,0 +1,64 @@
import {Vector2, Vector3} from 'math.gl';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Name file after primary export (calculate-tangents.js)?

for (let i = 0; i < length; i += 3) {
const locations = indices ? [indices[i], indices[i + 1], indices[i + 2]] : [i, i + 1, i + 2];
const [v0, v1, v2] = locations;
scratchPos0.set(positions[v0 * 3], positions[v0 * 3 + 1], positions[v0 * 3 + 2]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be nice with comments explaining each intermediary value

const scratchB1 = new Vector3();
const scratchB2 = new Vector3();

export function calculateTangents(positions, texCoords, indices) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

With out looking close, since we are not passing normals, I assume they are calculated from triangles. Would it make sense to pass in normals if we have them (to generate bitangents from crossing tangents and normals)

Since it is easy to calculate bitangents in shader, provide option for only calculating tangents?

indices
} = geometry;

const {tangents, bitangents} = calculateTangents(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice if calculateTangents accepted a luma.gl geometry object (looked for attributes and indices)

positions: new Buffer(
gl,
new Float32Array([
0.0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use // prettier-ignore to avoid such crazy autoformatting.

@@ -0,0 +1,22 @@
<!doctype html>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally think it is nicer to do these examples with an automatically generated HTML (html-webpack-plugin). But just follow the style of other examples in same repo.

<html>
<head>
<meta charset='UTF-8' />
<title>deck.gl w/ Esri ArcGIS API for JavaScript and I3S loader</title>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy pasta

@ibgreen ibgreen closed this Nov 1, 2021
@Pessimistress Pessimistress deleted the xx/tangent branch October 6, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants