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 bad texture coordinates in raster-12 #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix bad texture coordinates in raster-12 #20

wants to merge 1 commit into from

Conversation

dariuswiles
Copy link
Contributor

Problem Description

The Cube Model defines the triangle vertexes forming its top face in a different way to its other faces. It makes no difference for the demos where the cube faces are solid colors, but it is a problem in raster-12.html, when textures are introduced. It results in one of the top face triangles displaying the wrong part of the texture.

To see the problem, change the camera position on line 859 to:
var camera = Camera(Vertex(-1.5, 2, 3.5), MakeOYRotationMatrix(0));

You may also need to increase the ambient light on line 876 to better see the problem, e.g.:
Light(LT_AMBIENT, 0.6),

It's easier to see if you add code to rotate the camera around the X axis so you can move it above the cube and point it down.

For the record, raster-06.html to raster-09.html use the same vertex code; raster-10.html reorders the lines of the code block, but the vertexes are unchanged; and raster-11.html and raster-12.html use the same order as raster-06.html to raster-09.html, but change the vertexes for both triangles of the purple (top) cube face.

Fix in this PR

This PR fixes the problem by reordering the triangle vertexes on the top cube face. Although the problem is only visible on raster-12.html, the PR makes similar changes to raster-06.html to raster-12.html so the definition of cube vertexes remains consistent. One disadvantage to this approach is that eagle-eyed readers may notice that the triangles defining cube tops are drawn from different corners than shown in the figures in chapters 10 and 11.

Alternative Fix

If you prefer a smaller workaround rather than this fix, reject this PR and correct what I believe was an attempt at a workaround for just raster-12.html. It reorders the texture coordinates (not the vertexes), on line 844, but it unfortunately modifies the wrong triangle. Undo the bad workaround and implement it correctly by changing lines 843 and 844 to:

  Triangle([1, 0, 5], PURPLE, [Vertex( 0,  1,  0), Vertex( 0,  1,  0), Vertex( 0,  1,  0)], wood_texture, [Pt(1, 0), Pt(1, 1), Pt(0, 0)]),
  Triangle([5, 0, 4], PURPLE, [Vertex( 0,  1,  0), Vertex( 0,  1,  0), Vertex( 0,  1,  0)], wood_texture, [Pt(0, 0), Pt(1, 1), Pt(0, 1)]),

In case you are wondering how I found the problem, it was because I was puzzled why the texture coordinates on line 844 of raster-12.html didn't follow the pattern of the other triangles.

The Cube Model defines the triangle vertexes forming its top face in a
different way to its other faces. It makes no difference for the demos
where the cube faces are solid colors, but it is a problem in
raster-12.html, when textures are introduced. It results in one of the
top face triangles displaying the wrong part of the texture.

Fix the problem by reordering the triangle vertexes on the top cube
face. Although the problem is only visible on raster-12.html, make
similar changes to raster-06.html to raster-12.html so the definition
of cube vertexes remains consistent.
@ggambetta
Copy link
Owner

Hey, thanks for bringing this up!

I've looked at it a bit and my preferred solution would be to change raster-12 to

Triangle([4, 5, 1], PURPLE, [Vertex( 0,  1,  0), Vertex( 0,  1,  0), Vertex( 0,  1,  0)], wood_texture, [Pt(0, 0), Pt(1, 0), Pt(1, 1)]),
Triangle([4, 1, 0], PURPLE, [Vertex( 0,  1,  0), Vertex( 0,  1,  0), Vertex( 0,  1,  0)], wood_texture, [Pt(0, 0), Pt(1, 1), Pt(0, 1)]),

because it keeps the vertex order defined in the book (and the demos are easier to change than a book that has already been printed and shipped). The first 2 or 3 demos use the same ordering but the later one are a mess - I'll make them all similar to the ordering in the first 2 or 3.

Also note that this repo no longer has the latest version (ie the one on my website and the one that has been printed). I'd suggest following the website if you're not already doing that.

@dariuswiles
Copy link
Contributor Author

Your suggestion looks good to me! It definitely makes sense to keep the demos consistent with the book. Feel free to close this PR.

I had assumed the demos on GitHub were the same as on the website, so thanks for the heads up.

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.

2 participants