search

go-gl / mathgl

A pure Go 3D math library.
BSD 3-Clause "New" or "Revised" License
554 stars 65 forks source link

Fix TransformCoordinate #54

Closed der-antikeks closed 8 years ago

der-antikeks commented 8 years ago

Correction for a minor error. Should only affect perspective matrices.

dmitshur commented 8 years ago

Thanks for the fix @der-antikeks.

You'll want to do one more step in order to have the change apply to the mgl64 package as well, and that is run:

go generate github.com/go-gl/mathgl/mgl32

You can read https://github.com/go-gl/mathgl#contributing for more details.

der-antikeks commented 8 years ago

I have done this a while ago. Still missing something for a merge?

ghost commented 8 years ago

reviewing right now

ghost commented 8 years ago

I'm pretty sure that's right too, but could you link to some trustable source that explain the division by W ?

dmitshur commented 8 years ago

As I understand, the division by W was there before, it's just that its result was thrown away instead of being assigned to t. This PR fixes that unintended error.

LGTM.

I have done this a while ago. Still missing something for a merge?

Sorry, I missed it back then.

dmitshur commented 8 years ago

The documentation for that func (https://godoc.org/github.com/go-gl/mathgl/mgl32#TransformCoordinate) itself says:

This is effectively equivalent to the GLSL code

vec4 r = (m * vec4(v,1.));
r = r/r.w;
vec3 newV = r.xyz;
ghost commented 8 years ago

Yeah, but I can write anything in comments that doesn't mean it's true. I know that the statement before was effectively a noop. It would've been nice to link to like a trusted source that states how this works.

dmitshur commented 8 years ago

It would've been nice to link to like a trusted source that states how this works.

Sure. It can still be done.

Equally, if there's a trusted source saying this shouldn't be done, then we can revert this.

der-antikeks commented 8 years ago

In brief: To make perspective distortion work. Or to quote wikipedia:

After carrying out the matrix multiplication, the homogeneous component w will, in general, not be equal to 1. Therefore, to map back into the real plane we must perform the homogeneous divide or perspective divide by dividing each component by w.

A slightly longer but much more understandable explanation can be found here.