Re: More changes in path drawing

[Jiří Techet <techet gmail com>]

On Thu, Feb 17, 2011 at 02:25, Robert Park <rbpark exolucere ca> wrote:
> On Wed, Feb 16, 2011 at 2:39 AM, Jiří Techet <techet gmail com> wrote:
>> Paths should be drawn on idle now so it shouldn't be necessary to turn
>> off the path display before marker addition. As for the speed, we can
>> later try if a function taking a list of coordinates speeds up things
>> but as it is a minor change and there's no dramatic regression now, I
>> would concentrate on making a release as soon as possible. I expect
>> the performance will be slightly worse than in 0.8 -
>> ChamplainCoordinate is a gobject now so there is some overhead for
>> creating the object itself.
>
> Good news: Performance regression is almost totally fixed! 39,287
> points loaded in 12.7 seconds, and I watched the polygon render live
> as it loaded. Excellent! This is only half of it's former speed and
> I'm ok with that. I'm just glad it's no longer 22 minutes, I was quite
> worried for a while there ;-)
>
> Bad news: There is now severe visual corruption of the map during
> PathLayer drawing. See screenshot here:
>
> http://robru.github.com/Screenshot-GottenGeography.png
>
> Some notes:
>
> * the map renders just fine before I start adding nodes to the PathLayer
> * after the last call to add_node, if I navigate away from the
> displayed area, new areas load fine. if I navigate away until it's
> completely invisible, I can then navigate back and everything is fine.
> * It is only during the GPX load, when add_node is called rapidly,
> that this happens.
> * the PathLayer itself draws the path correctly, in the correct
> location, in spite of the map just showing garbage behind it.

Hi Robert,

I had a look at your python code and have a few comments:

1. I'm now pretty sure I know what's going wrong with the
set_position() not working correctly. The problem is this - there are
two functions ending with set_position() now:

void                clutter_actor_set_position          (ClutterActor *self,
                                                         gfloat x,
                                                         gfloat y);

void champlain_location_set_position (ChamplainLocation *location,
    gdouble latitude,
    gdouble longitude);

In python the prefix gets removed so they both become set_position().
ChamplainMarker inherits from ClutterActor and also implements
ChamplainLocation. So what happens in your code is that
clutter_actor_set_position() gets called instead of
champlain_location_set_position() because it's found first (interfaces
have probably lower priority). I bet if you use the "latitude" and
"longitude" properties, everything will work fine.

I fear that this would be confusing for every language supporting
inheritance. Therefore, I'll rename the set_position() function to
set_location() - after all the interface is called ChamplainLocation.

2. You don't initialize threads. Please do it - libsoup loads tiles on
background in separate threads and gobject has to be aware of that. In
the old python bindings we would call

gobject.threads_init()

before clutter initialization. Not sure about how it should be called now.

3. DON'T use clutter_actor_set_parent() unless you want to make your
life hard (clutter documentation says something very similar).
ClutterStage implements ClutterContainer interface so use
clutter_container_add_actor() instead. I'm pretty sure this will
resolve the animation problems of actors inserted into the stage.

4. Then there's this:

self.photo_layer = Champlain.MarkerLayer()

This calls the "default constructor" and initializes the selection
mode to CHAMPLAIN_SELECTION_NONE. Inside the add_marker method of the
layer there's the code

  champlain_marker_set_selectable (marker, mode != CHAMPLAIN_SELECTION_NONE);

that is, whatever you set before is reset according to the current
layer mode. I'm wondering whether your markers are selected correctly
because IMO the selection mode is reset once you insert the marker
into the layer.

This is about what I've noticed after quickly looking at your code.
The image corruption is strange and shouldn't happen - I'm now adding
clutter_actor_queue_redraw/relayout where necessary which could help
to fix some redrawing problems.

Jiri