Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot create more than one map on a page #154

Closed
jbeezley opened this issue Jul 11, 2014 · 16 comments
Closed

Cannot create more than one map on a page #154

jbeezley opened this issue Jul 11, 2014 · 16 comments

Comments

@jbeezley
Copy link
Contributor

In the following example, only the map on the right displays, while it responds to mouse events over both #map1 and #map2. There are similar issues with creating map, deleting the map, and creating a new map. The events from the deleted map are still being propagated in the new map. It seems like there are some properties in the gl renderer that are being shared globally. I'm not sure the extent to which it is possible to have two gl contexts on the same page, but at least we should make it possible to remove one cleanly and create another.

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <script src="//code.jquery.com/jquery-1.11.0.min.js"></script>
    <script src=//cdnjs.cloudflare.com/ajax/libs/d3/3.4.8/d3.min.js></script>
    <script src=//cdnjs.cloudflare.com/ajax/libs/gl-matrix/2.2.1/gl-matrix.js></script>
    <script src=//cdnjs.cloudflare.com/ajax/libs/proj4js/2.1.0/proj4.js></script>
    <script src=//cdnjs.cloudflare.com/ajax/libs/d3/3.4.8/d3.min.js></script>
    <script type="text/javascript" src="/web/lib/vgl.js"></script>
    <script type="text/javascript" src="/web/lib/geo.js"></script>
    <style>
        html, body{
            margin: 0;
            padding: 0;
            height: 100%;
            width: 100%;
            overflow: hidden;
        }
        .map {
            width: 50%;
            height: 100%;
        }

        #map1 {
            float: left;
        }

        #map2 {
            float: right;
        }
    </style>
    <script>

$(function(done) {


    var map1 = geo.map({'node': '#map1', 'zoom': 3}),
        map2 = geo.map({'node': '#map2', 'zoom': 3});
    map1.resize(0, 0, $('#map1').width(), $('#map1').height());
    map2.resize(0, 0, $('#map2').width(), $('#map2').height());
    map1.createLayer('osm');
    map2.createLayer('osm');

});

    </script>
</head>
<body>
<div id=map1 class=map></div>
<div id=map2 class=map></div>
</body>
@waxlamp
Copy link
Contributor

waxlamp commented Jul 11, 2014

Is this because of the singleton vglViewerInstance defined in src/gl/vglRenderer.js? I don't think there should be any problem with having multiple GL contexts in the same page - the API for getting GL contexts returns a distinct object for each canvas element, for example.

@aashish24
Copy link
Member

What is the error you are getting? No it won't be because of the vglViewerInstance. It would be because of the global var gl.

@aashish24
Copy link
Member

I thin its a vgl specific issue. Will push a fix on Monday (or on plane today). But if someone wants to take a look at it, then have a look at the variable gl. Basically, we have to localize it for each instance.

@waxlamp
Copy link
Contributor

waxlamp commented Jul 11, 2014

This must be the problem. Private variable GOOD, global variable BAD! :-P

@aashish24
Copy link
Member

Right.. unlike desktop gl, the webgl required making calls via this variable which complicated things just b bit. I think there two ways to handle this. We can enclose all the global vars if there are any into their own space (like what we do in jquery plugin) or pass the var to the gl objects. I am wondering what folks things about it

@aashish24
Copy link
Member

by enclosing globals, we will turn them into locals, so no globals.

@waxlamp
Copy link
Contributor

waxlamp commented Jul 11, 2014

I'm not sure what you mean about enclosing globals, but it seems to me that there should be a GL context either for each map object, or each GL layer. That way when draw calls are made within a [map | layer], the GL context local to that [map | layer] could be invoked directly.

@jbeezley
Copy link
Contributor Author

The singleton vglViewerInstance is definitely causing an issue here. It is only going to be instantiated once and attached to a canvas element it creates at this time. In the grits app, there is a case when the map is removed and the div element is deleted. Later, when the map is created again, this viewer will still be there and attached to a canvas that is no longer present.

On a related point, we have a private _exit function in the map, but no public method. We should expose this and make a unit test ensuring all of the resources and event handlers are freed.

@waxlamp
Copy link
Contributor

waxlamp commented Jul 11, 2014

Yeah, just from a software engineering standpoint, anything that is attached to a map should probably not be global or a singleton. "Viewers" and "contexts" seem like resources associated to one instance of a map, and as such should be created afresh and held privately by the map objects as needed.

@aashish24
Copy link
Member

I would prefer not to expose init and exit ... at least at the layer or below level. The idea is that parent container will do that for the child one.

@jbeezley
Copy link
Contributor Author

Yes, but who does it for map?

@aashish24
Copy link
Member

Two things... In my comment above that's why I said layer or below. I think when the map goes away js should garbage collect other objects. I have not seen any map API that has public exit method and that' why I would like to avoid it

@jbeezley
Copy link
Contributor Author

That is reasonable, but we need to make sure there are no globals hanging around preventing the garbage collection from occurring.

@jbeezley
Copy link
Contributor Author

@aashish24 Have you looked into this at all? It is causing problems in the GRITS application, and I'm not sure I am qualified to fix it.

@aashish24
Copy link
Member

I did some but didn't work on it much. I didn't realize its causing issues. I will look into this week and will try to get it done asap.

@jbeezley
Copy link
Contributor Author

Awesome! Thanks @aashish24.

@jbeezley jbeezley closed this as completed Jul 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants