GNOME Bugzilla – Bug 702570
cogl 1.16: Regression with event propagation in champlain
Last modified: 2013-07-01 12:43:07 UTC
Cogl commit 8f9151303 regresses libchamplain, causing an issue with event propagation with several layers. Adding a new reactive clutter actor to an upper layer makes it eat up all the mouse / keyboard events. Steps to reproduce: 1) Run champlain launcher-gtk.py demo: jhbuild build libchamplain jhbuild shell cd /path/to/checkout/libchamplain/demos ./launcher-gtk.py 2) The demo starts up and zooming works in the first screen 3) Click on Markers 4) Zooming no longer works Reverting the following commit makes it work again: commit 8f9151303d4d744205cce5cf0235b3e83e6687ef Author: Robert Bragg <robert@linux.intel.com> Date: Thu May 16 15:19:30 2013 +0100 pipeline: improve real_blend_enable checks
Thanks for the bug report. I did some exploration into this and one thing I noticed is that we are incorrectly enabling blending during the pick run after the markers are enabled. The following patch fixes that and does seem to fix the bug with libchamplain. However I don't understand why that causes a problem because the entire pick scene is rendered with solid colours so enabling blending shouldn't do any harm. It could make it unnecessarily slower but it shouldn't affect the results. The reason the problem is only triggered after the markers are displayed is that the pick region for the markers are drawn using a path. The entire pick scene is drawn using the global ‘opaque colour’ pipeline attached to the Cogl context. When it is created it is a copy of the root pipeline which is already marked as not needing blending. Normally the blending state on this pipeline would never be marked as dirty and would never get recalculated because only the colour ever changes on it. However when it is used to draw the path, it ends up getting a layer added to it because the primitive for the paths include texture coordinates. This causes a layer to be created when the attribute flushing code queries the unit for the layer. Making any layer changes on a pipeline causes it to recalculate the needs blending state considering every state flag, including the state for whether snippets are attached. The logic for determining whether snippets require blending was broken which explains why blending gets enabled. It'd be good to look into it a bit more to understand why enabling blending breaks it.
Created attachment 247420 [details] [review] Fix the check for whether snippets require pipeline to need blending When determining whether a pipeline needs blending, it was previously returning TRUE if the pipeline has no snippets, whereas it should be the other way around because we can't determine the final colour when there are snipets.
Comment on attachment 247420 [details] [review] Fix the check for whether snippets require pipeline to need blending Thanks, this patch looks good to land to me: Reviewed-by: Robert Bragg <robert@linux.intel.com>
Awesome, thanks! I have tested the patch and it fixes my test cases (both the libchamplain demo and gnome-maps).
I've pushed the patch to the master and cogl-1.16 branches. I think we should leave the bug open for now because I still don't understand why the patch fixes the bug.
Ok, I think I now understand why enabling blending broke it. When a layer is added to a pipeline without adding a texture then Cogl will internally end up binding a default 1x1 pixel white texture which will get modulated with the primary colour. This is the case in this example because using a path adds the layer in order to get the unit number for the texture coordinates which are contained in the path's primitive. For some reason this default texture has the alpha component set to zero, which is effectively an invalid pre-multiplied colour so the blending goes wrong.
Created attachment 247470 [details] [review] Fix the alpha value in the default texture data When a layer is added to a pipeline without setting a texture it ends up sampling from a default 1x1 texture which is meant to be solid white. However for some reason we were creating the texture with 0 opacity which is effectively an invalid premultiplied colour. This would make the blending behave oddly if it was used.
So I guess, another thing that this bug points out is that we pointlessly add a layer to the pipeline and start sampling from a texture whenever you draw with a path. We could fix that too with the following patch. Any of the three patches will independently fix the original bug.
Created attachment 247474 [details] [review] Don't create a layer when enabling texture coordinate attributes When a primitive is drawn with an attribute that contains texture coordinates Cogl will fetch the corresponding layer in order to determine the unit number. However if the pipeline didn't actually have a layer it would end up redundantly creating it. It's probably not a good idea to be modifying the pipeline while flushing the attributes state so this patch makes it pass the no-create flag to the get_layer function and then skips out enabling the attribute if the layer didn't already exist.
Comment on attachment 247470 [details] [review] Fix the alpha value in the default texture data Oops, this looks good to land to me
Comment on attachment 247474 [details] [review] Don't create a layer when enabling texture coordinate attributes This looks good to land to me, thanks
Thanks for the review. I've pushed both patches to all three branches.