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

Rename all instances of binormal as bitangent #4856

Merged
merged 4 commits into from
Jan 12, 2017

Conversation

shehzan10
Copy link
Member

@shehzan10 shehzan10 commented Jan 12, 2017

Fixes #4822

The PR deprecates GeometryPipeline.createBinormalAndTangent and recommends using the new GeometryPipeline.createTangentAndBitangent function.

The attributes have also been renamed to bitangent. This could cause some breakage.

This PR also makes the default value for DebugAppearance.perInstanceAttribute to false (was previously a required attribute).

Note: This PR fixes trailing whitespace in the files modified.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 12, 2017

@austinEng can you do the first review on this? Then I will do a final review and merge.

binormals[i + bottomOffset] = binormal.x;
binormals[i1 + bottomOffset] = binormal.y;
binormals[i2 + bottomOffset] = binormal.z;
bitangents[i + bottomOffset] = bitangent.x;
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace. This occurs several other times, but other than that, I think this looks good.

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've actually done that intentionally to have the code better aligned.

Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the eye for code formatting and cleanliness, but as a general rule we try to rely on auto-formatting (Webstorm CTRL-SHIFt-L) for blocks of code because it makes it more consistent across the code base as a whole. Unless we starting formatting array lookups like this everywhere, it's not ideal to only do it in a few places (or only have certain devs do it). A good mantra we try to follow is that the code should look like it was all written by one person, even though over 100 people have contributed so far.

That being said, in the future I'm hoping we can move to eslint and you that to enforce common style throughout the codebase. These types of rules would then become trivial to add or enforce.

* Breaking changes
* Removed separate `heading`, `pitch`, `roll` parameters from `Transform.headingPitchRollToFixedFrame` and `Transform.headingPitchRollQuaternion`. Pass a `headingPitchRoll` object instead. [#4843](https://github.com/AnalyticalGraphicsInc/cesium/pull/4843)
* The property `binornmal` has been renamed to `bitangent` throughout. [#4856](https://github.com/AnalyticalGraphicsInc/cesium/pull/4856)
Copy link
Contributor

Choose a reason for hiding this comment

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

Be more precise. Geometry, VertexFormat, etc. Otherwise, it isn't obvious to users where this occurs.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 12, 2017

Please submit an issue to fully remove the deprecated code. For an example, see #4730.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 12, 2017

In addition to the unit tests, do a bit of manual testing here. Open some of the geometry examples in Sandcastle, make sure to use a VertexFormat that includes tangent-space vectors, then visualize them with DebugAppearance and verify this branch is the same as master.

-1, 0, 0,
-1, 0, 0,
-0.4082482904638631, -0.8164965809277261, 0.4082482904638631], CesiumMath.EPSILON7);
});

it ('computeTangentAndBitangent computes tangent and bitangent for an BoxGeometry', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you fix this typo throughout the old test code: "an BoxGeometry'" -> "a BoxGeometry'"

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 12, 2017

Just those comments. Nice job!

@shehzan10
Copy link
Member Author

I have changed DebugAppearance.perInstanceAttribute to be false by default based on @pjcozzi's suggestion. Added this to the changelog as well.

@mramato Thanks for that heads up. I have reverted those spaces in this PR.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 12, 2017

Looks good. I also noticed this test failure in master, could you please open a separate PR to fix it?

Scene/EllipsoidPrimitive Constructs with options
DeveloperError: Expected scale to be typeof object, got number
Error
    at new DeveloperError (http://localhost:8080/Source/Core/DeveloperError.js:44:19)
    at Object.Check.typeOf.object (http://localhost:8080/Source/Core/Check.js:128:19)
    at Function.Matrix4.fromScale (http://localhost:8080/Source/Core/Matrix4.js:453:22)
    at http://localhost:8080/Specs/Scene/EllipsoidPrimitiveSpec.js:58:35
    at Object.<anonymous> (http://localhost:8080/Specs/spec-main.js:139:30)
    at attemptAsync (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1793:24)
    at QueueRunner.run (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1746:26)
    at QueueRunner.execute (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1732:10)
    at Spec.queueRunnerFactory (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:620:35)
    at Spec.execute (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:357:10)

@pjcozzi pjcozzi merged commit e440efc into CesiumGS:master Jan 12, 2017
@shehzan10 shehzan10 deleted the binormal-bitangent branch January 13, 2017 19:22
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.

Throughout rename "binormal" to "bitangent"
4 participants