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 triangle geometry (#2535) #2573

Merged
merged 5 commits into from
May 3, 2017
Merged

add triangle geometry (#2535) #2573

merged 5 commits into from
May 3, 2017

Conversation

andyli
Copy link
Contributor

@andyli andyli commented Apr 11, 2017

This adds a triangle geometry as discussed in #2535.
It is specified by 3 vertices, vertexA, vertexB, and vertexC.

If there is nothing need to amend regarding to property names, default values etc., I will send a PR to add docs.

@ngokevin
Copy link
Member

Most looks good, do you think it'd be better to center the triangle by default?

@andyli
Copy link
Contributor Author

andyli commented Apr 11, 2017

Yes, I think it make sense since the other geometries are centered by default.
I have just added a commit for that.

@ngokevin
Copy link
Member

Thanks! We can add docs, just a section in the geometry.md file works fine

@machenmusik
Copy link
Contributor

machenmusik commented Apr 11, 2017

vertexA/B/C seems a little verbose. i take it there is some problem preventing an array of vec3? if so, maybe we should change that - I am thinking something like vertices or verts as vec3 array would be better, and that would give use plumbing to support strips and other more advanced primitives?

@andyli
Copy link
Contributor Author

andyli commented Apr 12, 2017

@machenmusik
I've considered using an array of vec3 also, but I'm not sure. JS array doesn't have fixed length, so either we need additional check on array length during input, or have additional specification on how to handle longer/shorter array. I think using 3 (or even 4) vec3 variables instead of using array is still acceptable.

Building strip or other geometries of course is a different case, and I agree array would be more suitable when the vertex input amount is varied or very large.

Let me know if I've missed anything. :)

@machenmusik
Copy link
Contributor

for just basic triangle this is probably fine. my worry is that people will try to make thousands of these because they can't do strips and then wonder why performance isn't what it could be. so i naturally jump to syntax that also would work for strips etc.

@andyli
Copy link
Contributor Author

andyli commented Apr 12, 2017

make thousands of these because they can't do strips

People probably will, as well as thousands of boxes and spheres too ;)
I guess that means we have to add strip and other more advanced geometries...

For now, I will leave it as vertexA, vertexB, and vertexC. Documentation is added. Also found out my initial implementation has no UVs, so I added some code to compute a sensible set.

@andyli
Copy link
Contributor Author

andyli commented Apr 19, 2017

Rebased on top of master to trigger a TravisCI build.

@andyli
Copy link
Contributor Author

andyli commented Apr 25, 2017

Bump ;)

@ngokevin
Copy link
Member

ngokevin commented May 3, 2017

Thanks for the wait. I've updated the PR to not have to recreate the quaternion and vectors on each instance. Waiting for tests to pass then will merge!

@ngokevin ngokevin merged commit 43f6b26 into aframevr:master May 3, 2017
@andyli andyli deleted the triangle branch May 3, 2017 03:42
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.

4 participants