-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
@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; |
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.
Whitespace. This occurs several other times, but other than that, I think this looks good.
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.
I've actually done that intentionally to have the code better aligned.
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.
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) |
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.
Be more precise. Geometry
, VertexFormat
, etc. Otherwise, it isn't obvious to users where this occurs.
Please submit an issue to fully remove the deprecated code. For an example, see #4730. |
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 |
-1, 0, 0, | ||
-1, 0, 0, | ||
-0.4082482904638631, -0.8164965809277261, 0.4082482904638631], CesiumMath.EPSILON7); | ||
}); | ||
|
||
it ('computeTangentAndBitangent computes tangent and bitangent for an BoxGeometry', function() { |
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.
Can you fix this typo throughout the old test code: "an BoxGeometry'" -> "a BoxGeometry'"
Just those comments. Nice job! |
db6eaf4
to
89756c1
Compare
Looks good. I also noticed this test failure in master, could you please open a separate PR to fix it?
|
Fixes #4822
The PR deprecates
GeometryPipeline.createBinormalAndTangent
and recommends using the newGeometryPipeline.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
tofalse
(was previously a required attribute).Note: This PR fixes trailing whitespace in the files modified.