Getting the motionfactor stuff into the trunk

[Chris Foster-5]

Hi Trevor,

I've just been gathering the motionfactor commits together for final review
and inclusion into the trunk before the aqsis-1.6 release phase2.  The commits
are interleaved with some of your other work, so I'm using "git cherry-pick"
to pick them into a separate local motionfactor branch, after which I'll merge
them into the trunk when the review is done.  Here's what I've picked:

9d06e143
70693060
68d49a5e

Looking through all your patches I think these are the only important changes
related to motionfactor.  One thing I've noticed is that sometimes you
commit stuff from the motionfactor changes *together* with stuff from the
quadrics or curves work.  Please don't do this - it basically makes it almost
impossible for me to tease apart the changes into things I can review as
separate features.  Luckily the only times you've done this the motionfactor
changes have been trivial so I can pretty much ignore them.

However, we're not so lucky with the quadrics and curves work, which will
probably be impossible to separate.  Generally speaking, you should make a
commit whenever you have a set of self-contained changes.  It's far better for
a commit to be small but self-contained rather than large and mixed.

I'm going to do some testing of the functionality and review the code in a
subsequent mail to the list.

Cheers,
~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Chris Foster-5]

Gah, I meant to send this to the list too... forwarded now.


---------- Forwarded message ----------
From: Chris Foster <chris42f@...>
Date: Thu, Aug 13, 2009 at 11:05 AM
Subject: Re: Getting the motionfactor stuff into the trunk
To: Trevor Lovett <trevlovett@...>


On Thu, Aug 13, 2009 at 1:10 AM, Trevor Lovett<trevlovett@...> wrote:
> I didn't know the protocol for dealing with features.  Let me know the name
> of the motionfactor branch and if I need to add more changes to that code I
> can push to there.

There's no official protocol as such ;-)  But in general it's much
easier to deal with patches/commits which contain only a related set
of changes.

> For Quadrics + Curves: if it's a tangled mess on your end I don't mind doing
> a fresh checkout of aqsis_gsoc, reintegrating the changes (it's only a few
> files), and then pushing quadrics and curves to say, soc_quadrics,
> soc_curves.  Will that work for you?

I'm still learning about git and the possibilities which it provides
in terms of reviewing changes and getting them into the trunk, so bear
with me.  With svn, there's really not many choices about how to
integrate changes, and for people without svn access, sending a patch
in was really the only option.

With git it's possible to communicate whole sequences of commits via
email or via a separate repository like the gsoc one.  With regard to
just pushing things into new branches, I'm just a little concerned
about preserving the history.  Ideal commits should have at least the
following properties:
1) A minimal set of related changes that implements a single piece of
functionality
2) The changes should try to avoid leaving things in a broken state.

Of course, it's not always possible to achieve both of these, and IMHO
point (1) is somewhat more important than point (2) from the point of
view of a coherent development history.

By manually separating the quadrics and curves stuff and recommitting
in a big lump, we're loosing your commit history, but I'm not sure yet
whether to be worried about that.  If the changes turn out overall to
be fairly self-contained and not too large then maybe it's the best
thing to do.  Thoughts anyone?

~Chris

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Trevor Lovett]

Chris,

I'm at the point where I'm ready for you to review quadrics and MF, and possibly curves, once we sort out the git issues.  The quadrics passed the regression tests a few days ago and I think all that's lacking there is documentation and perhaps deletion of NaturalDice (shall I?).  MF probably requires more test cases other than petunia.rib and I'm ready for any bugfixes if they are needed.

About curves--we did find that bug that was exposed in the "simple.rib" I sent a few days ago.  My hunch is that ComputeTangent() is the problem and I will try to provide more convincing evidence if I can find it.  Other than that, I think that the work there is very close to being in a reviewable state.  We have a controllable attribute, support for cubic and linear hair-mode curves, and in my last commit you'll see I added support for dicing (lerping) user-specified params.  I'm not sure what sort of parameter in the rib I need to provide to test this--any suggestions? 

As far as splitting and recomitting: quadrics code is limited to quadrics.h / quadrics.cpp so that's easy,
MF is taken care of,
Curves is the messiest--it required changes to curves, cubiccurves, linearcurves, tokendictionary, attributes (I think) .h/.cpp and would cause the most pain, but I'm willing to do it as long as it gets everything into the review pipeline faster.

Trevor

On Thu, Aug 13, 2009 at 3:08 AM, Chris Foster <chris42f@...> wrote:

Gah, I meant to send this to the list too... forwarded now.

---------- Forwarded message ----------
From: Chris Foster <chris42f@...>
Date: Thu, Aug 13, 2009 at 11:05 AM
Subject: Re: Getting the motionfactor stuff into the trunk
To: Trevor Lovett <trevlovett@...>


On Thu, Aug 13, 2009 at 1:10 AM, Trevor Lovett<trevlovett@...> wrote:
> I didn't know the protocol for dealing with features.  Let me know the name
> of the motionfactor branch and if I need to add more changes to that code I
> can push to there.

There's no official protocol as such ;-)  But in general it's much
easier to deal with patches/commits which contain only a related set
of changes.

> For Quadrics + Curves: if it's a tangled mess on your end I don't mind doing
> a fresh checkout of aqsis_gsoc, reintegrating the changes (it's only a few
> files), and then pushing quadrics and curves to say, soc_quadrics,
> soc_curves.  Will that work for you?

I'm still learning about git and the possibilities which it provides
in terms of reviewing changes and getting them into the trunk, so bear
with me.  With svn, there's really not many choices about how to
integrate changes, and for people without svn access, sending a patch
in was really the only option.

With git it's possible to communicate whole sequences of commits via
email or via a separate repository like the gsoc one.  With regard to
just pushing things into new branches, I'm just a little concerned
about preserving the history.  Ideal commits should have at least the
following properties:
1) A minimal set of related changes that implements a single piece of
functionality
2) The changes should try to avoid leaving things in a broken state.

Of course, it's not always possible to achieve both of these, and IMHO
point (1) is somewhat more important than point (2) from the point of
view of a coherent development history.

By manually separating the quadrics and curves stuff and recommitting
in a big lump, we're loosing your commit history, but I'm not sure yet
whether to be worried about that.  If the changes turn out overall to
be fairly self-contained and not too large then maybe it's the best
thing to do.  Thoughts anyone?

~Chris

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Trevor Lovett]

Actually I think I know what might be going on with the curves bug--I'll report back in a bit.

On Thu, Aug 13, 2009 at 7:25 PM, Trevor Lovett <trevlovett@...> wrote:

Chris,

I'm at the point where I'm ready for you to review quadrics and MF, and possibly curves, once we sort out the git issues.  The quadrics passed the regression tests a few days ago and I think all that's lacking there is documentation and perhaps deletion of NaturalDice (shall I?).  MF probably requires more test cases other than petunia.rib and I'm ready for any bugfixes if they are needed.

About curves--we did find that bug that was exposed in the "simple.rib" I sent a few days ago.  My hunch is that ComputeTangent() is the problem and I will try to provide more convincing evidence if I can find it.  Other than that, I think that the work there is very close to being in a reviewable state.  We have a controllable attribute, support for cubic and linear hair-mode curves, and in my last commit you'll see I added support for dicing (lerping) user-specified params.  I'm not sure what sort of parameter in the rib I need to provide to test this--any suggestions? 

As far as splitting and recomitting: quadrics code is limited to quadrics.h / quadrics.cpp so that's easy,
MF is taken care of,
Curves is the messiest--it required changes to curves, cubiccurves, linearcurves, tokendictionary, attributes (I think) .h/.cpp and would cause the most pain, but I'm willing to do it as long as it gets everything into the review pipeline faster.

Trevor

On Thu, Aug 13, 2009 at 3:08 AM, Chris Foster <chris42f@...> wrote:

Gah, I meant to send this to the list too... forwarded now.

---------- Forwarded message ----------
From: Chris Foster <chris42f@...>
Date: Thu, Aug 13, 2009 at 11:05 AM
Subject: Re: Getting the motionfactor stuff into the trunk
To: Trevor Lovett <trevlovett@...>


On Thu, Aug 13, 2009 at 1:10 AM, Trevor Lovett<trevlovett@...> wrote:
> I didn't know the protocol for dealing with features.  Let me know the name
> of the motionfactor branch and if I need to add more changes to that code I
> can push to there.

There's no official protocol as such ;-)  But in general it's much
easier to deal with patches/commits which contain only a related set
of changes.

> For Quadrics + Curves: if it's a tangled mess on your end I don't mind doing
> a fresh checkout of aqsis_gsoc, reintegrating the changes (it's only a few
> files), and then pushing quadrics and curves to say, soc_quadrics,
> soc_curves.  Will that work for you?

I'm still learning about git and the possibilities which it provides
in terms of reviewing changes and getting them into the trunk, so bear
with me.  With svn, there's really not many choices about how to
integrate changes, and for people without svn access, sending a patch
in was really the only option.

With git it's possible to communicate whole sequences of commits via
email or via a separate repository like the gsoc one.  With regard to
just pushing things into new branches, I'm just a little concerned
about preserving the history.  Ideal commits should have at least the
following properties:
1) A minimal set of related changes that implements a single piece of
functionality
2) The changes should try to avoid leaving things in a broken state.

Of course, it's not always possible to achieve both of these, and IMHO
point (1) is somewhat more important than point (2) from the point of
view of a coherent development history.

By manually separating the quadrics and curves stuff and recommitting
in a big lump, we're loosing your commit history, but I'm not sure yet
whether to be worried about that.  If the changes turn out overall to
be fairly self-contained and not too large then maybe it's the best
thing to do.  Thoughts anyone?

~Chris

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Trevor Lovett]

If you reduce the width of the curve we used in simple.rib the cracking problem disappears.  One solution would be to approximate curvature (again) and where it's high, reduce the width slightly.  This would probably also get rid of that nasty kinking as well.  This is a very late-night non-mathematical analysis--I'll look into this more tomorrow. -t

On Thu, Aug 13, 2009 at 11:22 PM, Trevor Lovett <trevlovett@...> wrote:

Actually I think I know what might be going on with the curves bug--I'll report back in a bit.

On Thu, Aug 13, 2009 at 7:25 PM, Trevor Lovett <trevlovett@...> wrote:

Chris,

I'm at the point where I'm ready for you to review quadrics and MF, and possibly curves, once we sort out the git issues.  The quadrics passed the regression tests a few days ago and I think all that's lacking there is documentation and perhaps deletion of NaturalDice (shall I?).  MF probably requires more test cases other than petunia.rib and I'm ready for any bugfixes if they are needed.

About curves--we did find that bug that was exposed in the "simple.rib" I sent a few days ago.  My hunch is that ComputeTangent() is the problem and I will try to provide more convincing evidence if I can find it.  Other than that, I think that the work there is very close to being in a reviewable state.  We have a controllable attribute, support for cubic and linear hair-mode curves, and in my last commit you'll see I added support for dicing (lerping) user-specified params.  I'm not sure what sort of parameter in the rib I need to provide to test this--any suggestions? 

As far as splitting and recomitting: quadrics code is limited to quadrics.h / quadrics.cpp so that's easy,
MF is taken care of,
Curves is the messiest--it required changes to curves, cubiccurves, linearcurves, tokendictionary, attributes (I think) .h/.cpp and would cause the most pain, but I'm willing to do it as long as it gets everything into the review pipeline faster.

Trevor

On Thu, Aug 13, 2009 at 3:08 AM, Chris Foster <chris42f@...> wrote:

Gah, I meant to send this to the list too... forwarded now.

---------- Forwarded message ----------
From: Chris Foster <chris42f@...>
Date: Thu, Aug 13, 2009 at 11:05 AM
Subject: Re: Getting the motionfactor stuff into the trunk
To: Trevor Lovett <trevlovett@...>


On Thu, Aug 13, 2009 at 1:10 AM, Trevor Lovett<trevlovett@...> wrote:
> I didn't know the protocol for dealing with features.  Let me know the name
> of the motionfactor branch and if I need to add more changes to that code I
> can push to there.

There's no official protocol as such ;-)  But in general it's much
easier to deal with patches/commits which contain only a related set
of changes.

> For Quadrics + Curves: if it's a tangled mess on your end I don't mind doing
> a fresh checkout of aqsis_gsoc, reintegrating the changes (it's only a few
> files), and then pushing quadrics and curves to say, soc_quadrics,
> soc_curves.  Will that work for you?

I'm still learning about git and the possibilities which it provides
in terms of reviewing changes and getting them into the trunk, so bear
with me.  With svn, there's really not many choices about how to
integrate changes, and for people without svn access, sending a patch
in was really the only option.

With git it's possible to communicate whole sequences of commits via
email or via a separate repository like the gsoc one.  With regard to
just pushing things into new branches, I'm just a little concerned
about preserving the history.  Ideal commits should have at least the
following properties:
1) A minimal set of related changes that implements a single piece of
functionality
2) The changes should try to avoid leaving things in a broken state.

Of course, it's not always possible to achieve both of these, and IMHO
point (1) is somewhat more important than point (2) from the point of
view of a coherent development history.

By manually separating the quadrics and curves stuff and recommitting
in a big lump, we're loosing your commit history, but I'm not sure yet
whether to be worried about that.  If the changes turn out overall to
be fairly self-contained and not too large then maybe it's the best
thing to do.  Thoughts anyone?

~Chris

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Trevor Lovett]

Actually the curve-cracking problem is probably more basic than this.  Chris (or others):  should anything strange happen when two grids overlap in screen space?  To see what I mean you can try setting the curve width very high in simple.rib (like 0.5).

And you can ignore all but the first response to your email.

On Fri, Aug 14, 2009 at 1:02 AM, Trevor Lovett <trevlovett@...> wrote:

If you reduce the width of the curve we used in simple.rib the cracking problem disappears.  One solution would be to approximate curvature (again) and where it's high, reduce the width slightly.  This would probably also get rid of that nasty kinking as well.  This is a very late-night non-mathematical analysis--I'll look into this more tomorrow. -t

On Thu, Aug 13, 2009 at 11:22 PM, Trevor Lovett <trevlovett@...> wrote:

Actually I think I know what might be going on with the curves bug--I'll report back in a bit.

On Thu, Aug 13, 2009 at 7:25 PM, Trevor Lovett <trevlovett@...> wrote:

Chris,

I'm at the point where I'm ready for you to review quadrics and MF, and possibly curves, once we sort out the git issues.  The quadrics passed the regression tests a few days ago and I think all that's lacking there is documentation and perhaps deletion of NaturalDice (shall I?).  MF probably requires more test cases other than petunia.rib and I'm ready for any bugfixes if they are needed.

About curves--we did find that bug that was exposed in the "simple.rib" I sent a few days ago.  My hunch is that ComputeTangent() is the problem and I will try to provide more convincing evidence if I can find it.  Other than that, I think that the work there is very close to being in a reviewable state.  We have a controllable attribute, support for cubic and linear hair-mode curves, and in my last commit you'll see I added support for dicing (lerping) user-specified params.  I'm not sure what sort of parameter in the rib I need to provide to test this--any suggestions? 

As far as splitting and recomitting: quadrics code is limited to quadrics.h / quadrics.cpp so that's easy,
MF is taken care of,
Curves is the messiest--it required changes to curves, cubiccurves, linearcurves, tokendictionary, attributes (I think) .h/.cpp and would cause the most pain, but I'm willing to do it as long as it gets everything into the review pipeline faster.

Trevor

On Thu, Aug 13, 2009 at 3:08 AM, Chris Foster <chris42f@...> wrote:

Gah, I meant to send this to the list too... forwarded now.

---------- Forwarded message ----------
From: Chris Foster <chris42f@...>
Date: Thu, Aug 13, 2009 at 11:05 AM
Subject: Re: Getting the motionfactor stuff into the trunk
To: Trevor Lovett <trevlovett@...>


On Thu, Aug 13, 2009 at 1:10 AM, Trevor Lovett<trevlovett@...> wrote:
> I didn't know the protocol for dealing with features.  Let me know the name
> of the motionfactor branch and if I need to add more changes to that code I
> can push to there.

There's no official protocol as such ;-)  But in general it's much
easier to deal with patches/commits which contain only a related set
of changes.

> For Quadrics + Curves: if it's a tangled mess on your end I don't mind doing
> a fresh checkout of aqsis_gsoc, reintegrating the changes (it's only a few
> files), and then pushing quadrics and curves to say, soc_quadrics,
> soc_curves.  Will that work for you?

I'm still learning about git and the possibilities which it provides
in terms of reviewing changes and getting them into the trunk, so bear
with me.  With svn, there's really not many choices about how to
integrate changes, and for people without svn access, sending a patch
in was really the only option.

With git it's possible to communicate whole sequences of commits via
email or via a separate repository like the gsoc one.  With regard to
just pushing things into new branches, I'm just a little concerned
about preserving the history.  Ideal commits should have at least the
following properties:
1) A minimal set of related changes that implements a single piece of
functionality
2) The changes should try to avoid leaving things in a broken state.

Of course, it's not always possible to achieve both of these, and IMHO
point (1) is somewhat more important than point (2) from the point of
view of a coherent development history.

By manually separating the quadrics and curves stuff and recommitting
in a big lump, we're loosing your commit history, but I'm not sure yet
whether to be worried about that.  If the changes turn out overall to
be fairly self-contained and not too large then maybe it's the best
thing to do.  Thoughts anyone?

~Chris

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Chris Foster-5]

Hi Trevor,

On Fri, Aug 14, 2009 at 3:25 AM, Trevor Lovett<trevlovett@...> wrote:
> I'm at the point where I'm ready for you to review quadrics and MF, and
> possibly curves, once we sort out the git issues.  The quadrics passed the
> regression tests a few days ago and I think all that's lacking there is
> documentation and perhaps deletion of NaturalDice (shall I?).  MF probably
> requires more test cases other than petunia.rib and I'm ready for any
> bugfixes if they are needed.

Something which has been confusing me is the fact that for petunia.rib,
*increasing* the motionfactor attribute from 1 to 4 makes the scene render
much slower.  Luckily I managed to figure out what this is, and it turns out
to be a bug in the trunk in somewhat unrelated code.  I've fixed it in the
trunk and updated the gsoc respository with the fixed version (on the master
branch).  The upshot is that increasing the shading rate by increasing the
motionfactor attribute should always make the rendertime shorter now.

As for rendertime results, locally with the motionfactor optimization I'm
seeing something like 25% speedups which is quite respectable!

I've had a quck look at the final motionfactor code, and it mostly looks ok
but I think there's still a few minor niggles I'd like cleaned up.
Unfortunately it's just too late here for me to do a proper coherent review
now, but I'll get onto it first thing tomorrow.  We should have this stuff in
the trunk by Monday so that it can go into aqsis-1.6.  Google also have their
official coding deadline on the 17th, but this appears to be 1 week eariler
than the really hard deadline for end-of-project reviews (due 24th IIRC) so we
have an extra week of time left from that point of view.

Some test cases which are needed for MF are:
1) Check that behaviour is sensible for multiple motion segments (2 or more)
2) Check that the scaling constant is a good size so that quality doesn't
   suffer too much compared to the non-MF case.
3) Check that speedups are consistent with what we expect from increasing
   the shading rate by hand.

... and possibly one or two more which I've forgotten since it's too
late here ;-)

> As far as splitting and recomitting: quadrics code is limited to quadrics.h
> / quadrics.cpp so that's easy,
> MF is taken care of,
> Curves is the messiest--it required changes to curves, cubiccurves,
> linearcurves, tokendictionary, attributes (I think) .h/.cpp and would cause
> the most pain, but I'm willing to do it as long as it gets everything into
> the review pipeline faster.

I'll let you know about these within the next few days.

~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Trevor Lovett]

Ok thanks for the response.  I need to let you know that Monday I effectively don't exist because I will be moving to a new flat in Norway, so I'd like to get all the official stuff done and trunked by sunday evening.

On Fri, Aug 14, 2009 at 5:42 PM, Chris Foster <chris42f@...> wrote:

Hi Trevor,

On Fri, Aug 14, 2009 at 3:25 AM, Trevor Lovett<trevlovett@...> wrote:
> I'm at the point where I'm ready for you to review quadrics and MF, and
> possibly curves, once we sort out the git issues.  The quadrics passed the
> regression tests a few days ago and I think all that's lacking there is
> documentation and perhaps deletion of NaturalDice (shall I?).  MF probably
> requires more test cases other than petunia.rib and I'm ready for any
> bugfixes if they are needed.

Something which has been confusing me is the fact that for petunia.rib,
*increasing* the motionfactor attribute from 1 to 4 makes the scene render
much slower.  Luckily I managed to figure out what this is, and it turns out
to be a bug in the trunk in somewhat unrelated code.  I've fixed it in the
trunk and updated the gsoc respository with the fixed version (on the master
branch).  The upshot is that increasing the shading rate by increasing the
motionfactor attribute should always make the rendertime shorter now.

As for rendertime results, locally with the motionfactor optimization I'm
seeing something like 25% speedups which is quite respectable!

I've had a quck look at the final motionfactor code, and it mostly looks ok
but I think there's still a few minor niggles I'd like cleaned up.
Unfortunately it's just too late here for me to do a proper coherent review
now, but I'll get onto it first thing tomorrow.  We should have this stuff in
the trunk by Monday so that it can go into aqsis-1.6.  Google also have their
official coding deadline on the 17th, but this appears to be 1 week eariler
than the really hard deadline for end-of-project reviews (due 24th IIRC) so we
have an extra week of time left from that point of view.

Some test cases which are needed for MF are:
1) Check that behaviour is sensible for multiple motion segments (2 or more)
2) Check that the scaling constant is a good size so that quality doesn't
  suffer too much compared to the non-MF case.
3) Check that speedups are consistent with what we expect from increasing
  the shading rate by hand.

... and possibly one or two more which I've forgotten since it's too
late here ;-)

> As far as splitting and recomitting: quadrics code is limited to quadrics.h
> / quadrics.cpp so that's easy,
> MF is taken care of,
> Curves is the messiest--it required changes to curves, cubiccurves,
> linearcurves, tokendictionary, attributes (I think) .h/.cpp and would cause
> the most pain, but I'm willing to do it as long as it gets everything into
> the review pipeline faster.

I'll let you know about these within the next few days.

~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Chris Foster-5]

On Fri, Aug 14, 2009 at 3:25 AM, Trevor Lovett<trevlovett@...> wrote:
> I'm at the point where I'm ready for you to review quadrics and MF, and
> possibly curves, once we sort out the git issues.  The quadrics passed the
> regression tests a few days ago and I think all that's lacking there is
> documentation and perhaps deletion of NaturalDice (shall I?).  MF probably
> requires more test cases other than petunia.rib and I'm ready for any
> bugfixes if they are needed.

Regarding the quadrics NaturalDice, definitely go ahead and delete it,
provided the regression tests still pass.

Here's what's hopefully a final review of the motionfactor code.  If you can
just make the changes in your branch that will be fine - as long as the
commits aren't mixed up with commits regarding the curves or quadrics ;-)

> diff --git a/libs/core/bound.h b/libs/core/bound.h
> index 3ff7f16..d652574 100644
> --- a/libs/core/bound.h
> +++ b/libs/core/bound.h
> @@ -115,7 +115,8 @@ class CqBound
>   }
>
>   CqBound& operator=( const CqBound& From );
> -
> +
> + void getBoundCuboid(CqVector3D cuboid[8]);

Docs here please (move them from the implmentation file).

>   void Transform( const CqMatrix& matTransform );
>   void Encapsulate( const CqBound* const bound );
>   void Encapsulate( const CqVector3D& v );
> diff --git a/libs/core/geometry/surface.cpp b/libs/core/geometry/surface.cpp
> index 3280b74..061a559 100644
> --- a/libs/core/geometry/surface.cpp
> +++ b/libs/core/geometry/surface.cpp
> @@ -716,6 +716,98 @@ TqFloat CqSurface::AdjustedShadingRate() const
>   const TqFloat areaRatio = 0.025;
>   shadingRate *= max(1.0f, areaRatio*focusFactor*minCoC*minCoC);
>   }
> +
> + // Adjust shadingRate based on motionfactor
> +
> + //get motionfactor variable from rib
> + const TqFloat* motionFactor = m_pAttributes->GetFloatAttribute("System", "GeometricMotionFactor");
> + TqFloat motionFac = motionFactor[0];
> +
> + if (motionFac > 0.0)
> + {
> + //adjust me
> + const TqFloat scalingConst = 0.05;

Is this the value you've settled on?  If so, comments should describe how you
arrived at the magic constant.  If you haven't done extensive testing, don't
worry about it too much - I can run some more tests on this once it's in the
trunk and document what I find.

> + // only proceed if object is moving
> + if (keyframeTimes.size() > 1)
> + {
> + CqBound objBound(m_Bound);
> + CqMatrix matCameraToObject0;
> + context->matSpaceToSpace("camera", "object", NULL, pTransform().get(), *keyframeTimes.begin(), matCameraToObject0);
> + objBound.Transform(matCameraToObject0);
> +
> + CqVector3D objBoundCuboid[8];
> + objBound.getBoundCuboid(objBoundCuboid);

objBoundCuboid doesn't seem to be used anywhere... best delete it.

> +
> + TqFloat minSpeed = FLT_MAX; // initialize to max float value
> + CqVector3D prevWorldCuboid[8];
> + CqVector3D currWorldCuboid[8];
> +
> + // get bounding box (cuboid) for time 0
> + CqBound prevWorldBound(objBound);
> + CqMatrix matObjectToCameraT0;
> + context->matSpaceToSpace("object", "camera", NULL, pTransform().get(), keyframeTimes[0], matObjectToCameraT0);
> + prevWorldBound.Transform(matObjectToCameraT0);
> + prevWorldBound.getBoundCuboid(prevWorldCuboid);
> +
> + for (int t = 1; t < static_cast<int>(keyframeTimes.size()); t++)
> + {
> + // get cuboid for time t
> + CqBound currWorldBound(objBound);
> + CqMatrix matObjectToCameraT1;
> + context->matSpaceToSpace("object", "camera", NULL, pTransform().get(), keyframeTimes[t], matObjectToCameraT1);
> + currWorldBound.Transform(matObjectToCameraT1);
> + currWorldBound.getBoundCuboid(currWorldCuboid);

Ok, this looks much better now.

> + // this takes vector differences between verts of cuboids C(t1) and C(t0) and finds min distance^2
> + TqFloat minDist = FLT_MAX;
> + for (int i = 0; i < 8; i++)
> + {
> + CqVector3D vecDiff = currWorldCuboid[i] - prevWorldCuboid[i];
> + TqFloat dist = vecDiff.Magnitude2();
> +
> + if (dist < minDist)
> + {
> + minDist = dist;
> + }
> + }
> +
> + // calculate speed of primitive, keep track of min-speed
> + float speed = std::sqrt(minDist) / (keyframeTimes[t] - keyframeTimes[t-1]);
> + if (speed < minSpeed) {
> + minSpeed = speed;
> + }
> +
> + // update prevCuboid (lame copy for now):
> + for (int i = 0; i < 8; i++) {
> + prevWorldCuboid[i] = currWorldCuboid[i];
> + }

Minor style niggle regarding the brace placement in these two for loops:
please put the opening brace on a separate line to conform with the rest of
the code ;-)  I think it's best to insist on uniformity with this one, sorry!

Alternatively, the following is also nice for a single-line loop body:

for (int i = 0; i < 8; i++)
        prevWorldCuboid[i] = currWorldCuboid[i];

> + }
> +
> + // multiply shading rate by distance factor, user-specified motion-factor and special const
> + TqFloat shadingRateAdj = max<float>(1.0, scalingConst * minSpeed * motionFac);

There's a point I made already in a past review: minSpeed has the wrong units
here.  Ultimately you want to estimate some sort of distance travelled during
the shutter interval, so minSpeed should be multiplied by the amount of time
that the shutter is open to get a measure of distance.

Obviously if the shutter is open for infinitely short time, the shading rate
shouldn't be adjusted, since everything will be perfectly sharp and
non-blurred.

> + shadingRate *= shadingRateAdj;
> + }
> + }
> +
>   return shadingRate;
>  }
>

Apart from that, there's a lot of spaces/tabs past the end of line which I'd
prefer to see gone if possible.  git diff --color with the appropriate
revision range will show them to you prominently.

Thanks!
~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Chris Foster-5]

I've just been having some trouble applying your latest changes from your
branch to do with the motionfactor stuff, and relaised that I reviewed the
wrong version of your code: I accidentally left out the changes from revision
171c1fb which introduced the isMoving() functions... sorry!

Unfortunately it turns out that in your last two motionfactor commits there's
some mixture between the code which was changed for the quadrics or curves and
the motionfactor code.  I've just spent a while cherry-picking the changes and
hand resolving the conficts that appear, but it's hard to tell if all is
correct :-/  I'll have to work out the details another day - hopefully
tomorrow - before merging into the trunk.

~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Trevor Lovett]

Hm sorry that it's been so much trouble. I can do a fresh check out of aqsis_gsoc and reintegrate the quadrics stuff, create a new branch and push.  Might be less error prone.  Let me know.

On Sun, Aug 16, 2009 at 6:33 PM, Chris Foster <chris42f@...> wrote:

I've just been having some trouble applying your latest changes from your
branch to do with the motionfactor stuff, and relaised that I reviewed the
wrong version of your code: I accidentally left out the changes from revision
171c1fb which introduced the isMoving() functions... sorry!

Unfortunately it turns out that in your last two motionfactor commits there's
some mixture between the code which was changed for the quadrics or curves and
the motionfactor code.  I've just spent a while cherry-picking the changes and
hand resolving the conficts that appear, but it's hard to tell if all is
correct :-/  I'll have to work out the details another day - hopefully
tomorrow - before merging into the trunk.

~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Chris Foster-5]

On Mon, Aug 17, 2009 at 2:37 AM, Trevor Lovett<trevlovett@...> wrote:
> Hm sorry that it's been so much trouble.

That's ok.  I'm just learning about git too.

> I can do a fresh check out of
> aqsis_gsoc and reintegrate the quadrics stuff, create a new branch and
> push.  Might be less error prone.

That would be good I think, please do so!  In particular, I'm first
trying to get the motionfactor stuff fixed up for aqsis-1.6 release
phase 2 so that's a priority for me at the moment.

Can you create a new branch soc_optimization_cleanup (or something)
off the current master branch, and recommit your changes there.  I
think you can compact all the motionfactor commits together into one
commit since they're all closely related.  Please make sure you
* Write a descriptive commit message
* Very carefully avoid mixing in anything unrelated - a good way to do
this is by reading the diffs carefully before you commit; I find
that's a good practise anyway when writing the commit message.

If you could do the same thing with the quadrics changes when you have
time that would be most appreciated.  There's probably no need for a
separate branch from the motionfactor cleanup, provided you're careful
about keeping the commits separate.

Since there's still some work to go with the curves, perhaps we could
consolidate the curves changes so far and start in a fresh branch
without any mixture with the quadrics work?

Thanks!
~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Trevor Lovett]

Pushed motionfactor (wow it actually does use quite a few files--no wonder you were confused) and the quadrics code in separate commits.  Ran petunia.rib and got expected results.  Quadrics passed regressions.

When I get some form of internet access at my new flat I will scrub the curves work a bit and then push into the new branch I've been using: soc_optimization_cleanup .  If you see something amiss in that normal calculation please let me know.  Thanks.

Trevor

On Sun, Aug 16, 2009 at 6:52 PM, Chris Foster <chris42f@...> wrote:

On Mon, Aug 17, 2009 at 2:37 AM, Trevor Lovett<trevlovett@...> wrote:
> Hm sorry that it's been so much trouble.

That's ok.  I'm just learning about git too.

> I can do a fresh check out of
> aqsis_gsoc and reintegrate the quadrics stuff, create a new branch and
> push.  Might be less error prone.

That would be good I think, please do so!  In particular, I'm first
trying to get the motionfactor stuff fixed up for aqsis-1.6 release
phase 2 so that's a priority for me at the moment.

Can you create a new branch soc_optimization_cleanup (or something)
off the current master branch, and recommit your changes there.  I
think you can compact all the motionfactor commits together into one
commit since they're all closely related.  Please make sure you
* Write a descriptive commit message
* Very carefully avoid mixing in anything unrelated - a good way to do
this is by reading the diffs carefully before you commit; I find
that's a good practise anyway when writing the commit message.

If you could do the same thing with the quadrics changes when you have
time that would be most appreciated.  There's probably no need for a
separate branch from the motionfactor cleanup, provided you're careful
about keeping the commits separate.

Since there's still some work to go with the curves, perhaps we could
consolidate the curves changes so far and start in a fresh branch
without any mixture with the quadrics work?

Thanks!

~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development