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

Fix face.color is undefined in SoftwareRender.drawTriangle #8036

Merged
merged 1 commit into from
Jan 29, 2016

Conversation

daoshengmu
Copy link
Contributor

Resolve bug #7997

mrdoob added a commit that referenced this pull request Jan 29, 2016
Fix face.color is undefined in SoftwareRender.drawTriangle
@mrdoob mrdoob merged commit 1287f84 into mrdoob:dev Jan 29, 2016
@mrdoob
Copy link
Owner

mrdoob commented Jan 29, 2016

Thanks!

@@ -713,7 +713,8 @@ THREE.SoftwareRenderer = function ( parameters ) {
mpUV12 = new THREE.Vector2(),
mpUV23 = new THREE.Vector2(),
mpUV31 = new THREE.Vector2(),
tempFace = { vertexNormalsModel : [] };
tempFace = { vertexNormalsModel : []
, color : { r: face.color.r, g: face.color.b, b: face.color.b } };
Copy link
Contributor

Choose a reason for hiding this comment

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

@daoshengmu you could perhaps just do color: face.color, since it is not modified, therefor saving some memory?

Copy link
Contributor

Choose a reason for hiding this comment

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

in fact, a bunch of new THREE.Vector-s above could also be done once somewhere outside, probably...

@daoshengmu
Copy link
Contributor Author

@makc. Yep, tempFace = { vertexNormalsModel : [], color : face.color }; should be better. I will send another PR to fix this. As you mentioned, a bunch of new THREE.Vector-s is not good for performance. In the previous version of SoftwareRenderer that I modified, the performance issue is happened there. I can give a help to this part as well.

@daoshengmu daoshengmu deleted the fixFaceColorInSoftware branch January 31, 2016 03:28
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.

3 participants