[libavg-devel] Re: Div Node Rotations Patch

[Ulrich von Zadow]

Hi Nick,

I've tested your patch here - all tests pass. I added another test which
checks if nonrectangular clipping regions work (have a look at
rotate3.avg) and that one passes, too :-). I've checked your patch and
the additional test into the branch and updated the branch to current trunk.

Still, there's a few minor nitpicks and one major thing that needs to
change before this can be merged to trunk. The major issue is that - as
I said in my previous email - Node::calcAbsViewport() is fundamentally
flawed now and should really disappear along with Node::m_AbsViewport.
The calculations in there simply do not make sense for rotated nodes,
and they'll confuse people who need to maintain the code later on.
You'll probably want to use glTranslate so that opengl works in the
local coordinate system of the current node and then just blt everything
in local coordinates.

Also, a heads-up that I'll disappear on Sept. 4 and be away for all of
September. I'll mostly be out of reach of email during that time, much
less be able to compile things.

More comments inline...

Nick Hebner schrieb:
> Well after a busy few days, I have finally found some time to work on 
> this again. I have found that the problem that manifested itself after 
> the words test is related to the change of resolution that this test 
> caused. All of the other tests used the same resolution except 
> testWords, and for some reason this caused problems with later tests. 
> The  only thing that I have found to fix this is to delete the display 
> engine on Player::stop() (shutting down the entire SDL system), and 
> recreate it on Player::play(). I don't really like this, but it seems to 
> work for now.

Hm. I don't like that either, for two reasons. The first is that we're
probably hiding a state bug in that way (I assume that either OpenGL
state or SDLEngine state is broken at the end of playback), and the
second is that the tests now take twice as long to execute.

[...]
> Image::obscures(),
> VideoBase::obscures(),
> PanoImage::obscures() - this function relies on getVisibleRect to 
> determine if the current node is within another node's visible rect. As 
> it is, this can still fail to return strictly correct results if a node 
> is obscured by multiple nodes, leading to an extraneous redraw of that 
> node. With rotations, this only gets worse as now the test is more than 
> a simple Rect::contains. I can see three alternatives:
> 1. Write a Point DisplayEngine::applyRotations(Point) function to rotate 
> the four corners of the rect, and check if they are contained by the 
> node in question.
> 2. Draw nodes from front to back, using the depth buffer to allow the 
> proper pixels through.
> 3. Continue drawing back to front, and disregard obscuring completely 
> (possibly leading to some extraneous node draws).
> 
> The third is the easiest to accomplish, and is the route that I took in 
> this patch.

That's perfectly ok to get things working, but obscures() allows for
more optimizations than just reducing overdraw - for instance, videos
don't upload images to the graphics card if nobody can see them anyway.
Front-to-back drawing is something that is probably going to become a
necessity when we start experimenting with shaders, but it can't replace
obscures(). So, transforming the four corners of the rect into local
coords is what really needs to happen. There is already a
Node::getRelPos(AbsPos) - how about adding Node::getAbsPos(RelPos)? Both
of these functions should just involve a recursive application of
translations and rotations.

> The attached patch includes the changes discussed here. It passes all 
> current tests, and I have added a few more rotated clipping tests.

Nice :-).

Anyway - thanks for your tenacity. We're getting there.

Cheers,

   Uli

-- 

Ulrich von Zadow | +49-172-7872715 | cocacoder at jabber.berlin.ccc.de