Merge lp:~grigorig/nux/bug-927441 into lp:nux/2.0

[greg]

This fixes wrong texture coordinates, which cause bug #927441

This fixes wrong texture coordinates, which cause bug #927441. See the bug for more information.

[Jay Taoko (jaytaoko)]

Can you elaborate what is wrong with the computation? I have seen the bug you reported on some GPUs (mostly old intel GPUs and some AMD GPUs). Newer Intel GPUs (Sandy Bridge I believe) don't show the bug. Neither does NVidia GPUs.
What the computation is doing is matching pixel texture coordinates with the center of texels. Right now I am seeing inconsistent result which I am trying to match to an error in the code or a problem with some drivers.

[Andrea Cimitan (cimi)]

I have sandybridge Jay, and I have the "Dash does not render panel line" issue. Not affected by "Far left character in panel (and launcher popups) distorted"

[greg (grigorig)]

> What the computation is doing is matching pixel texture coordinates with the
> center of texels. Right now I am seeing inconsistent result which I am trying
> to match to an error in the code or a problem with some drivers.

You don't need to do that. The fragments already use centered coordinates, the texture coordinate interpolation done by the GPU ensures that. You're actually doing the opposite, and the calculation moves the sampling point to the boundary between two texels, and that is why it behaves so irregularly: depending on rounding and internal precision in the GPU, different pixels are selected.

You can force nux rendering to GL_LINEAR sampling to visualize this issue. The rendering becomes quite blurry, which is a pretty clear sign that texels are not sampled at the center - with centered sampling positions linear is equal to nearest. I've posted a screenshot to demonstrate that to bug #927441. With my patch, the rendering result is equal for linear and nearest.

[Jay Taoko (jaytaoko)]

Thanks for the feedback. I realise I have applied a texel-center correction that is typical of DirectX 9 (for correct texture-pixel alignment in 2D screen rendering).

review: Approve

[greg (grigorig)]

I don't really want to push, but why is this not merged yet? The merge proposal has been marked as approved for two days. This is a simple and straight-forward bug fix (and in fact it restores the old code in Nux 1.0, as I can see now), the problem has high visibility and affects many users. Why is bug #927441 marked fixed, yet the issue clearly happens with Nux master? I do not understand the development practices at hand here.

[Mikkel Kamstrup Erlandsen (kamstrup)]

greg, Nux branches are landed by a bot once the bot detects that the branch has entered Approved state. Either the bot is hanging for some reason, or it is holding back the branch because we're in beta freeze. I assure you no malicious intents :-)

FWIW: We're reviewing the practice of freezing code trunks; you're not the first one to wonder. It has some benefits wrt to packaging, but as you observe, also some drawbacks wrt to the development.

[Didier Roche-Tolomelli (didrocks)]

@greg, mikkel: all branches are built automatically into a 12.04 chroot before being merged to trunk. If it doesn't build, the branch is rejected.
On Friday evening, precise pbuilder was broken to ssl connection, so if I would have released the lock by then, the branch would have been rejected automatically. Maybe you would have prefered that?

Now 12.04 is fine again and the bot can restart merging, so no need to start a holly war Mikkel (and please, contribute to the thread about freezing as nothing was decided upon it, isn't it?) ;)

[greg (grigorig)]

@didrocks
Of course I would not have preferred a reject. :) I just found the ways things were unfolding very confusing.

=== modified file 'NuxGraphics/RenderingPipe.cpp'
--- NuxGraphics/RenderingPipe.cpp	2012-01-17 03:57:31 +0000
+++ NuxGraphics/RenderingPipe.cpp	2012-03-21 20:47:18 +0000
@@ -207,23 +207,21 @@
       if (texxform.m_tex_coord_type == TexCoordXForm::OFFSET_SCALE_COORD)
       {
         // Scale and offset the texture coordinates.
-        // Ensure precise alignment between pixels and the texture coordinates
         // With a scale of 1.0 for both uscale and vscale, the texture is scaled over the entire surface of the quad.
         // If the texture and the quad have the same size, then the texture is mapped 1 to 1 with the pixels of the quad.
-        texxform.u0 = texxform.uoffset + texxform.uscale * (tex_width / (float)quad_width)   * (1.0f / (2.0f * tex_width));
-        texxform.v0 = texxform.voffset + texxform.vscale * (tex_height / (float)quad_height) * (1.0f / (2.0f * tex_height));
-        texxform.u1 = texxform.uoffset + texxform.uscale * (tex_width / (float)quad_width)   * (2.0f * quad_width - 1.0f) / (2.0f * tex_width);
-        texxform.v1 = texxform.voffset + texxform.vscale * (tex_height / (float)quad_height) * (2.0f * quad_height - 1.0f) / (2.0f * tex_height);
+        texxform.u0 = texxform.uoffset;
+        texxform.v0 = texxform.voffset;
+        texxform.u1 = texxform.uoffset + texxform.uscale * (tex_width / (float)quad_width)   * quad_width / tex_width;
+        texxform.v1 = texxform.voffset + texxform.vscale * (tex_height / (float)quad_height) * quad_height / tex_height;
       }
       else if (texxform.m_tex_coord_type == TexCoordXForm::OFFSET_COORD)
       {
         // Offset the texture coordinates but preserve the proportion of the of the texture over the quad.
         // If the texture size is smaller than the quad, it will be tiled over it.
-        // Ensure precise alignment between pixels and the texture coordinates
-        texxform.u0 = texxform.uoffset + (1.0f / (2.0f * tex_width));
-        texxform.v0 = texxform.voffset + (1.0f / (2.0f * tex_height));
-        texxform.u1 = texxform.uoffset + (2.0f * quad_width - 1.0f) / (2.0f * tex_width);
-        texxform.v1 = texxform.voffset + (2.0f * quad_height - 1.0f) / (2.0f * tex_height);
+        texxform.u0 = texxform.uoffset;
+        texxform.v0 = texxform.voffset;
+        texxform.u1 = texxform.uoffset + quad_width / tex_width;
+        texxform.v1 = texxform.voffset + quad_height / tex_height;
       }
       else if (texxform.m_tex_coord_type == TexCoordXForm::UNNORMALIZED_COORD)
       {