unity-dev team mailing list archive

[Sam Spilsbury <smspillaz@xxxxxxxxx>]

Hey All,

I know that refactoring our existing code to improve readability and
design is important, but I've seen a few regressions slip through our
projects lately because we've been doing it too aggressively. For
example, I just spent the last few days scratching my head trying to
figure out why nux::CreateTexture2DFromID always returned black
textures and went through multiple opengl debugging tools to find this
change:

-  ObjectPtr<IOpenGLTexture2D> GpuDevice::CreateTexture2DFromID(int id
+  ObjectPtr<IOpenGLTexture2D> GpuDevice::CreateTexture2DFromID(int /* id */
     , int width
     , int height
     , int levels
     , BitmapFormat pixel_format
     , NUX_FILE_LINE_DECL)
   {
-    IOpenGLTexture2D *ptr;
-    ptr = new IOpenGLTexture2D(width, height, levels, pixel_format,
true, NUX_FILE_LINE_PARAM); // ref count = 1;
-    ptr->_OpenGLID = id;
-    ObjectPtr<IOpenGLTexture2D> h = ObjectPtr<IOpenGLTexture2D>
(ptr); // ref count = 2
-    ptr->UnReference(); // ref count = 1
-    return h;
+    GpuInfo gpu_info = GetGpuInfo();
+    int msz = gpu_info.GetMaxTextureSize();
+    if(width <= 0 || height <=0 || width > msz || height > msz)
+    {
+      return ObjectPtr<IOpenGLTexture2D>();
+    }
+
+    ObjectPtr<IOpenGLTexture2D> result;
+    result.Adopt(new IOpenGLTexture2D(width, height, levels,
pixel_format, true, NUX_FILE_LINE_PARAM));
+    return result;
   }

The refactoring and changes here make sense - using managed pointers
and also sanity checking the foreign texture. However, there's one
critical line that's missing, which causes the whole thing to break:

-    ptr->_OpenGLID = id;

More frighteningly, the compiler must have warned about this (unused
parameter), and the solution taken was to simply comment out the
parameter name:

-  ObjectPtr<IOpenGLTexture2D> GpuDevice::CreateTexture2DFromID(int id
+  ObjectPtr<IOpenGLTexture2D> GpuDevice::CreateTexture2DFromID(int /* id */

Be careful when making changes like these, especially when the
compiler is warning that you are no longer using data you once did. In
addition, when doing refactoring, please try and do either of the
following:

1. Add tests for the unit under test (manual test do not count, and
use coverage tools to make sure your tests actually run the code you
think they do)
2. Write a sample program excersizing the system's behaviour with that
function (this applies especially to toolkits)
3. Run the system under test with a coverage build to make sure that
the changes you've made are actually being excersized.

The other thing to consider is that when writing code which has that
one crucial line required to tie the whole thing together, put a
comment there to say what it does so that other people don't
accidentally remove it.

Cheers,

Sam
-- 
Sam Spilsbury