kicad-developers team mailing list archive

[mj <imp@xxxxxxxxxxxxxxx>]

On 03.08.2014 10:14, jp charras wrote:
> Le 03/08/2014 03:24, Martin Janitschke a écrit :
>> Heyho,
>> please feel free to test and apply the attached patch for the 3D viewer.
>>
>> It'll enable anti-aliasing - if the renderer supports it with up to
>> factor 8 (beyond this there was no difference in the subtracted images).
>> The code also deals with the possible changes to the attributes array
>> (avoiding fixed indices).
>>
>> For the results see http://imgur.com/a/TgaXT .
>>
>> Bye,
>> imp
> 
> Thanks for your patch.
> 
> However it has a potential bug in loops:
> for( ii = 0; ii < sizeof( attrs ); ii += 2)
> does not stop the iteration at the end of the table.
Ah, good catch, forgot / sizeof( attrs[0] )...

> and the code expects  WX_GL_SAMPLES is the last item in attrs table,
> which is currently true, but could be modified later (and there is no
> comment to explain this constraint).
No. It _is_ build to avoid depending on special positions / indices in
that array - that's why there are 2 for loops searching for those
elements. But it's expected to be on an even position in that array, see
below.

The attrs array is:
attribute-id, value for that attribute,
another attribute-id, value for another attribute,
...., 0. (Or at least what I've thought it would be first).
That's the reason for the += 2 (skipping the value of the attribute we
checked). BUT wxwidgets made some strage exceptions for "boolean
attributes" that don't have a value (likely copying it from opengl <
13...). This can lead to an exception of the 2*n + 1 rule. Added a
comment for this and put in those attributes in a second time as padding
(thus enforcing the tuples / attributes on even positions).
Additionally we can change the condition to
"ii < sizeof( attrs ) / sizeof( attrs[0] ) - 1" and save 2x one
iteration (avoiding to check the null termination against WX_GL_...).
New patch attached.

Bye,
imp

=== modified file '3d-viewer/3d_frame.cpp'
--- 3d-viewer/3d_frame.cpp	2014-07-30 15:39:55 +0000
+++ 3d-viewer/3d_frame.cpp	2014-08-03 09:50:27 +0000
@@ -107,8 +107,62 @@
     ReCreateMainToolbar();
 
     // Make a EDA_3D_CANVAS
-    int attrs[] = { WX_GL_RGBA, WX_GL_DOUBLEBUFFER, WX_GL_DEPTH_SIZE, 16,
-                    WX_GL_STENCIL_SIZE, 1, 0 };
+    int attrs[] = { // This array is should be 2*n+1 - sadly wxwidgets / glx < 13 allowed
+                    // a thing named "boolean attributes" that don't take a value.
+                    // (See src/unix/glx11.cpp -> wxGLCanvasX11::ConvertWXAttrsToGL() ).
+                    // To avoid problems due to this, just specify those attributes twice.
+                    // Only WX_GL_RGBA, WX_GL_DOUBLEBUFFER, WX_GL_STEREO are such boolean
+                    // attributes.
+
+                    // Boolean attributes (using itself at padding):
+                    WX_GL_RGBA, WX_GL_RGBA, 
+                    WX_GL_DOUBLEBUFFER, WX_GL_DOUBLEBUFFER,
+
+                    // Normal attributes with values:
+                    WX_GL_DEPTH_SIZE, 16,
+                    WX_GL_STENCIL_SIZE, 1,
+                    WX_GL_SAMPLE_BUFFERS, 1, // Enable multisampling support (antialiasing).
+                    WX_GL_SAMPLES, 0, // Disable AA for the start.
+                    0 }; // NULL termination
+
+    unsigned int ii;
+
+    // Check if the canvas supports multisampling.
+    if( EDA_3D_CANVAS::IsDisplaySupported( attrs ) )
+    {
+        // Check for possible sample sizes, start form the top.
+        int maxSamples = 8; // Any higher doesn't change anything.
+        int samplesOffset = 0;
+
+        for( ii = 0; ii < sizeof( attrs ) / sizeof( attrs[0] ) - 1; ii += 2 )
+        {
+            if( attrs[ii] == WX_GL_SAMPLES )
+            {
+                samplesOffset = ii+1;
+                break;
+            }
+        }
+        
+        attrs[samplesOffset] = maxSamples;
+
+        for( ; maxSamples > 0 && !EDA_3D_CANVAS::IsDisplaySupported( attrs );
+            maxSamples = maxSamples>>1 )
+        {
+            attrs[samplesOffset] = maxSamples;
+        }
+    }
+    else
+    {
+        // Disable multisampling
+        for( ii = 0; ii < sizeof( attrs ) / sizeof( attrs[0] ) - 1; ii += 2 ){
+            if( attrs[ii] == WX_GL_SAMPLE_BUFFERS )
+            {
+                attrs[ii+1] = 0;
+                break;
+            }
+        }
+    }
+
     m_canvas = new EDA_3D_CANVAS( this, attrs );
 
     m_auimgr.SetManagedWindow( this );