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

Hard normals #173

Merged
merged 33 commits into from
Nov 2, 2016
Merged

Hard normals #173

merged 33 commits into from
Nov 2, 2016

Conversation

tfili
Copy link

@tfili tfili commented Oct 10, 2016

@lasalvavida Just opening a pull request here so we can keep track of what needs to be done to get this in.

CC @pjcozzi

@lasalvavida
Copy link
Contributor

lasalvavida commented Oct 14, 2016

This is ready for review. Adds support for stripping off existing normals with -r and generating face normals with -f. I've run 2CylinderEngine through and below are some screenshots to highlight the difference between the existing smooth normals and the added hard normals.

Smooth Normals Hard Normals
2-cylinder-smooth-normals 2-cylinder-hard-normals

@lasalvavida
Copy link
Contributor

Note: the material generation in generateNormals may be changed or removed entirely when I finish adding model material common generation, but for now I think that it is acceptable.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 15, 2016

Thanks @lasalvavida.

@tfili once you confirm that this meets your needs, @lilleyse can review and merge.

@tfili
Copy link
Author

tfili commented Oct 22, 2016

@lasalvavida this looks pretty good. The one issue we saw was you can't run this twice in a row because it either generates an invalid technique or we are missing an if check when processing a technique.

@tfili
Copy link
Author

tfili commented Oct 25, 2016

@lasalvavida After a little digging the culprit seems to be the remove normals step. It removes the technique from the material so it can be regenerated, but there are stages before that happens that expect the technique to exist.

@lasalvavida
Copy link
Contributor

Okay, I should have some time tomorrow to take a look at this.

@lasalvavida
Copy link
Contributor

This should be fixed now

@lasalvavida lasalvavida mentioned this pull request Oct 28, 2016
@lilleyse
Copy link
Contributor

@tfili let me know when this is all good from your end and I'll do a review.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 31, 2016

@tfili let me know when this is all good from your end and I'll do a review.

@tfili is this ready for review?

@tfili
Copy link
Author

tfili commented Nov 1, 2016

@pjcozzi @lilleyse This looks good to me.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 1, 2016

@lilleyse please merge when you are happy with the code.

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Nice job! The hard normals look good.

dragon

removeBufferViews(gltf);
removeBuffers(gltf);
mergeBuffers(gltf);
mergeDuplicateVertices(gltf);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since mergeDuplicateVertices is the next step in Pipeline we can remove it from here.

@@ -0,0 +1,9 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file used?

lilleyse and others added 2 commits November 2, 2016 14:20
@tfili
Copy link
Author

tfili commented Nov 2, 2016

@lilleyse I fixed you last 2 suggestions. Let me know if there is anything else.

@lilleyse
Copy link
Contributor

lilleyse commented Nov 2, 2016

Thanks @tfili that should be it.

@lilleyse lilleyse merged commit 1afbb94 into master Nov 2, 2016
@lilleyse lilleyse deleted the hard-normals branch November 2, 2016 18:38
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.

4 participants