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

geometry.translate -> pivot component in extras/ (fixes #1341) #1339

Merged
merged 1 commit into from
Apr 12, 2016

Conversation

ngokevin
Copy link
Member

@ngokevin ngokevin commented Apr 4, 2016

Description:

geometry.translate doesn't work with BufferGeometry, and the property was always awkward. The feature also made the geometry component more complex to maintain. A lot of examples use this feature, but we want to remove this feature.

Replacing it with a pivot component that is a helper for wrapping it within another entity. This might be useful for the editor to change the pivot point without having to create/expose additional entities.

I'm putting it extras/ for now, but it has a case for being a standard component. While it could be done by wrapping it within another entity, we might not want to create an extra entity just to change the pivot point and abstract that completely.

Difference between the pivot and geometry.translate is that rather than translating the entity, pivot will internally move the pivot point. This is done by translating the entity and untranslating it at the parent level. The effect is that pivot vectors are the opposite of translate vectors (e.g, -1 -2 -3 vs 1 2 3) and since it doesn't translate, you don't have to manually untranslate using position.

Possible caveat is that since we untranslate the pivot using position, a future update to position might mess things up. We would have to make the position component aware of the pivot component. Right now, the pivot component works best on static objects and you can use a wrapper entity for future updates to position.

Changes proposed:

  • Pivot component (that internally moves the pivot point)
  • Remove geometry.translate (which translated the entity while keeping the pivot point)
  • Update examples to use pivot over translate
  • Lot of examples did not need translate/pivot, updated those

@ngokevin ngokevin changed the title [WIP] geometry.translate -> pivot component in extras/ geometry.translate -> pivot component in extras/ Apr 5, 2016
@ngokevin ngokevin changed the title geometry.translate -> pivot component in extras/ geometry.translate -> pivot component in extras/ (fixes #1341) Apr 6, 2016
@dmarcos
Copy link
Member

dmarcos commented Apr 6, 2016

The main reason to have pivots was being able to change the origin of a loaded model. Now that models are a separate component we could make translate a property of the model components.

@ngokevin
Copy link
Member Author

ngokevin commented Apr 7, 2016

We were previously using translate to change the pivot point of meshes for animations. Many of the animation examples were using it to change the anchor of the animation. Not with just models, you might want to change the pivot point for a group of meshes.

Granted that could be done with a wrapper entity. Though we might want to abstract that. In 3D modelling, setting the pivot point doesn't create another object. It just does it internally, so it would be nice to match that expectation.

@@ -41,20 +41,20 @@

<a-entity id="dots1" rotation="0 0 45">
<a-animation attribute="scale" from="1 0 1" to="1 0.6 1" begin="800" dur="750" fill="both" easing="ease-out"></a-animation>
<a-cylinder position="0 2 0" radius="0.04" height="2" translate="0 -1 0" color="#EF2D5E" open-ended="true" side="double">
Copy link
Member

Choose a reason for hiding this comment

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

I haven't tried but removing translation should change the animation of this demo. Does this look all right?

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, I couldn't tell the difference.


// Transfer rotation to outer group.
originalGroup.rotation.set(0, 0, 0);
outerGroup.rotation.set(originalRotation.x, originalRotation.y, originalRotation.z);
Copy link
Member

Choose a reason for hiding this comment

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

what about:

outerGroup.rotation.copy(originalGroup.rotation);
originalGroup.rotation.set(0, 0, 0);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants