#415 PatchMesh motion blur error

non_fatal
closed
7
2012-09-20
2011-06-27
No

PatchMesh fails to render correctly when motion blur is applied (see attached RIB file).

Discussion

  • Jonathan Merritt

    Bug example.

     
  • Jonathan Merritt

    In the RIB file, comment out the motion block and try one or other of the PatchMesh objects individually to see the bug.

    Since this is a failure on a basic primitive, I've bumped the priority up a little.

     
  • Jonathan Merritt

    OK - this looks quite simple:

    When a PatchMesh is created, Aqsis immediately splits it into primitive Patch objects. These patch objects are then passed to CreateGPrim, which incorrectly assumes that they are multiple instances of a single Patch, because they are received within a motion block.

     
  • Jonathan Merritt

    Patch with fix is attached (details in commit message).

     
  • Chris Foster

    Chris Foster - 2011-07-04

    This patch looks pretty good Jonathan, I absolutely agree with the general
    strategy. There is a problem though: it fails on some regression tests, in
    particular, regression tests containing the Teapot primitive (Geometry
    "teapot") which makes use of patch meshes internally.

    Apart from that, I have some nits:

    • Indenting should be four spaces per tabstop (it's also preferred to use tabs
      in files which currently use tabs for indenting... which isn't all of them
      though, ugh.)

    • It seems to me that SplitInternally shouldn't be a public function since it
      only needs to be called once and directly before ConvertToBezierBasis(). Why
      not make it private and call it internally function?

    • I can also see some places where you call m_patches.clear() where it seems
      unnecessary.

    Having said all that I think it's probably close :)
    ~Chris

     
  • Jonathan Merritt

    Patch (second attempt)

     
  • Jonathan Merritt

    Hi Chris,

    A modified patch is attached, featuring:

    1. Better indentation (hopefully I've followed the Aqsis conventions now).
    2. No regression failures (as far as I can tell - pdiff still says some images are different even though they look the same to me). The teapot primitive has been modified slightly to make sure it calls ConvertToBezierBasis().
    3. SplitInternally() is no longer a public method, and is now called internally from within ConvertToBezierBasis().
    4. No extraneous m_patches.clear() or m_patches.reserve() calls.

    Jonathan.

     
  • Chris Foster

    Chris Foster - 2011-07-05

    Hi Jonathan,

    This looks really good now, thanks for fixing those extra things. I've committed it, along with adding the test case to the RTS.

    The only remaining nit was a small stylistic issue of bracket placement which I fixed myself:

    if(blah)
    {
    // yada yada
    }

    rather than

    if(blah) {
    // yada yada
    }

    I've nothing against the second style (in fact, it's been growing on me recently) but most of the codebase uses the former so I made that small change before committing.

    You're right about the state of the RTS, there's a bunch of bogus failures which we need to address at some stage, ugh.

    ~Chris

     


Anonymous

Cancel  Add attachments





Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks