Vector feature with external image / background image artifact on map setCenter

This issue is partly resolved by applying the following patch: http://trac.openlayers.org/attachment/ticket/1675/1675-r7782-A1.patch

With it, the externalGraphic is properly cleared from the map, however a backgroundGraphic set on the vector feature is not.

Example using the patch: http://t2gnow.com/1709/sundials-osm.html

I was able to resolve the backgroundGraphic issue by adding the following code in the file OpenLayers/Renderer/Elements.js, after applying the patch above:

    drawGeometry: function(geometry, style, featureId) {
        ...
        if (style.display != "none") {
            this.redrawNode(geometry.id, geometry, style, featureId); 
        } else {
            var node = OpenLayers.Util.getElement(geometry.id);
            if (node) {
                node.parentNode.removeChild(node);
            }
            
>            node = OpenLayers.Util.getElement(geometry.id+this.BACKGROUND_ID_SUFFIX);
>            if (node) {
>                node.parentNode.removeChild(node);
>            }
        }
    },

Thanks for the catch. This fix is absolutely valid, not only after applying the patch for #1675.

If you are able, can you please create a patch with that fix (against trunk, using svn diff or diff -u)? You would also have to sign a Contributors License Agreement (see http://trac.openlayers.org/wiki/HowToContribute).

Thanks!

Attached the patch 1709-r7900-A0.patch with the fix above for the background graphic

Just thinking: calling OpenLayers.Util::getElement is quite expensive, so we do not want to do that too often. So I would suggest to do this only if we have a style.backgroundGraphic. This all could go in the

if (style.backgroundGraphic) {
    this.redrawBackgroundNode(geometry.id, geometry, style, featureId);
}

block (L467-469), so it would look something like this:

if (style.backgroundGraphic) {
    if (style.display != "none") {
        this.redrawBackgroundNode(geometry.id, geometry, style, featureId);
    } else {
        node = OpenLayers.Util.getElement(geometry.id + this.BACKGROUND_ID_SUFFIX);
        if (node) {
            node.parentNode.removeChild(node);
        }
    }
}

This would also give us another performance improvement, by not having to call redrawBackgroundNode if we have a display: "none" style.

I agree completely, that's a much better solution. In fact, it's what my original attempt to fix the issue for this ticket looked like, however it naturally didn't accomplish what I wanted with the 1675-r7782-A1.patch applied because of the following in Renderer.js:

    drawFeature: function(feature, style) {
        if(style == null) {
            style = feature.style;
        }
        if (feature.geometry) {
            if (!feature.geometry.getBounds().intersectsBounds(this.extent)) {
                style = {display: "none"};
            }
            return this.drawGeometry(feature.geometry, style, feature.id);
        }
    },

Now if instead of setting style like we are there, we did something more like this:

                style = OpenLayers.Util.extend({}, style);
                style.display = "none";

It would work out and my background graphics would be properly hidden. I know that's maybe sort of outside the scope of this issue, but just as a heads up.

I'll attach a new patch with the check for background graphic.

You're right, I was not thinking about that. Since copying the whole style element may also be expensive, we could rely on the stored _style on the node, something like that:

        if (style.display != "none") {
            this.redrawNode(geometry.id, geometry, style, featureId);
            if (style.backgroundGraphic) {
                this.redrawBackgroundNode(geometry.id, geometry, style, featureId);
            } 
        } else {
            node = OpenLayers.Util.getElement(geometry.id);
            if (node) {
                if (node._style.backgroundGraphic) {
                    node.parentNode.removeChild(document.getElementById(
                        geometry.id + this.BACKGROUND_ID_SUFFIX));
                }
                node.parentNode.removeChild(node);
            }
        }

The more I think about it, the more I am convinced that the above solution is the correct one: If we have a node when style.display=="none", then the node has already be drawn, and will have a node._style hash attached. If this _style hash has a backgroundGraphic property set, we know that we have to remove a background graphic node, because this is still there from the previous drawGeometry call.

jstern81: Sorry for the inconvenience with all the change requests around this patch. Just let me know if you don't want to continue working on it, then I can take care of finishing it.

Nice, that seems like the best solution so far. I made one small tweak from the above code for the latest patch: I'm redrawing the background node before the foreground one.

ahocevar: don't worry about it, I'm happy to help out and this way I'm actually learning a bit more than I would have otherwise. But on the other hand if there's any other small changes and it's more convenient for you to just make the change without waiting around for me, please feel free to do so. I'll still be hanging around to see what the final change looks like. cheers

(In [7919]) Let's take care that background graphic nodes of features with backgroundGraphic style get removed properly when the style is changed to display: "none".

This solves the backgroundGraphic issue of #1709, the remaining issue with externalGraphic will be fixed by #1675.

Thanks jstern81 for the patch, and I am truly impressed by jstern81 finding the right spot in the codebase that needs to be fixed -- as an OpenLayers newcomer.

Tests added by myself. r=me. (references #1709)

r7930 /trunk/openlayers/ (12 files in 7 dirs): New vector rendering for better performance and less renderer specific limitations. r=elemoine (closes #1675, closes #1656, closes #1631, closes #1431, closes #1709)