Proper config.js format

jsilvestre commented 12 years ago

Hey

I've been thinking about defining a proper config.js.

Currently it doesn't take into account localization and some information are redundant (like marker type informations). An example is probably better than any explanation.

Metadata = {
    "icons_path" : "assets/images/icons/32x32/",
    "config_version" : number-generated-by-the-backend,
    "date_generation" : date-generated-by-the-backend
};

// We let areas like they are

Markers.explore = {
    'data_translation' : {
      'en' : { 'name' : 'Explore' },
      'fr' : { 'name' : "Exploration" }
   },
   'markerTypes' : [
       {'slug' : 'hearts',
       'icons' : 'hearts.png',
        'data_translation' : {
         'en' : { 'name' : 'Hearts' },
         'fr'  : { 'name' : 'Coeur' }
         },
         'markers' : [ { 'id' : xxx, 'lat' : xxx, 'lng' : xxx, 'data_translation' : { 'en' : { 'title' : "blabla", 'desc' : 'blibli' } } } ]
      }
  ]   

};

The data_translation parts are obviously not complete but we would put whatever we want in it because this is extensible. So what we are discussing here https://github.com/lpdumas/gw2cartographers/issues/35 could find its place inside.

I'm looking forwards to hearing from your ideas!

lpdumas commented 12 years ago

I love it! so if we had what we've talk on #35 we get:

Metadata = {
    "icons_path" : "assets/images/icons/32x32/",
    "config_version" : number-generated-by-the-backend,
    "date_generation" : date-generated-by-the-backend
};

Markers.explore = {
  'data_translation' : {
    'en' : { 'name' : 'Explore' },
    'fr' : { 'name' : "Exploration" }
  },
  'markerTypes' : [
    {
      'slug' : 'hearts',
      'icons' : 'hearts.png',
      'dTitle' : 0,
      'dDesc' : 0,
      'dWikiLink' : 0,
      'data_translation' : {
        'en' : { 'name' : 'Hearts' },
        'fr'  : { 'name' : 'Coeur' }
      },
      'markers' : [ { 'id' : xxx, 'lat' : xxx, 'lng' : xxx, 'data_translation' : { 'en' : { 'title' : "blabla", 'desc' : 'blibli' } } } ]
    }
  ]   

};

I don't know if we are missing something, but i think that's pretty solid ;)

jsilvestre commented 12 years ago

I would see title/desc/wiki link in data_translation since they are going to be localized. The idea is that all the fields that would be localized are in the data_translation field. Also, I am for putting fields only when they are needed. Ie: no dTitle, dDesc, dWikiLink for hearts, and no data_translation in the marker for hearts.

I need to go for the day I'll explain further tonight or tomorrow.

lpdumas commented 12 years ago

oups, you're right ;) so :

Metadata = {
    "icons_path" : "assets/images/icons/32x32/",
    "config_version" : number-generated-by-the-backend,
    "date_generation" : date-generated-by-the-backend
};

Markers.explore = {
  'data_translation' : {
    'en' : { 'name' : 'Explore' },
    'fr' : { 'name' : "Exploration" }
  },
  'markerTypes' : [
    {
      'slug' : 'hearts',
      'icons' : 'hearts.png',
      'data_translation' : {
        'en' : { 'name' : 'Hearts', 'dTitle' : "blablabla", 'dDesc' : "blablabla", 'dWikiLink' : "blablabla"},
        'fr'  : { 'name' : 'Coeur', 'dTitle' : "blablabla", 'dDesc' : "blablabla", 'dWikiLink' : "blablabla" }
      },
      'markers' : [ { 'id' : xxx, 'lat' : xxx, 'lng' : xxx, 'data_translation' : { 'en' : { 'title' : "blabla", 'desc' : 'blibli' } } } ]
    }
  ]   

};

Wouldn't data_translation be required for hearts markers? I mean description, title and wikilink are going to change from each marker to another. On the other hand, marker for pets won't need it.

jsilvestre commented 12 years ago

That's what I meant : data_translation inside the marker for hearts etc. and inside the "markerType" section for pets etc. But when it is not needed in the marker (like for pets), we don't put any data. Same thing with data_translation field inside the "markerType" section. We only put data when it is needed. What do you think ?

edit: I haven't made a real example in JS yet but this is the raw data in JSON I use to test gw2cbackend:

{
"trainers":{
    "name":"Trainers",
    "markerTypes":[
        {
            "slug":"trainer_ranger",
            "translated_data" : {
                "en" : {
                    "name" : "Ranger",
                    "desc" : "A trainer for rangers.",
                    "wikiLink" : "http://gw2.wiki.com/en/ranger"
                },
                "fr" : {
                    "name" : "Rôdeur",
                    "desc" : "Un entraîneur pour les rôdeurs",
                    "wikiLink" : "http://gw2.wiki.com/fr/ranger"
                }    
            },
            "markers":[{"id": 1, "lng":-45.636962890625,"lat":29.439597566602927}]},
        {
            "slug":"trainer_necro",
            "translated_data" : {
                "en" : {
                    "name" : "Necromancer",
                    "desc" : "A trainer for necromancers.",
                    "wikiLink" : "http://gw2.wiki.com/en/necromancer"
                },
                "fr" : {
                    "name" : "Nécromant",
                    "desc" : "Un entraîneur pour les nécromant",
                    "wikiLink" : "http://gw2.wiki.com/fr/necromander"
                }    
            },
            "markers":[]
        }
    ]
},

"explore":{
    "name":"Explore",
    "markerTypes":[
        {
        "name":"Hearts",
        "slug":"hearts",
        "markers":[
            {"id":2,"lng":-44.384765625,"lat":29.19053283229458,

                "translated_data" : {
                    "en" : {
                        "name" : "Help farmer Diah",
                        "desc" : "Help Diah by watering corn.",
                        "wikiLink" : "http://gw2.wiki.com/en/blabla"
                    },
                    "fr" : {
                        "name" : "Aider le fermier Diah",
                        "desc" : "Aider Diah en arrosant le maïs.",
                        "wikiLink" : "http://gw2.wiki.com/fr/blabla"
                    }    
                }},
            {"id":3,"lng":-46.636962890625,"lat":29.439597566602927,

                "translated_data" : {
                    "en" : {
                        "name" : "Help farmer George",
                        "desc" : "",
                        "wikiLink" : ""
                    },
                    "fr" : {
                        "name" : "Aider le fermier George",
                        "desc" : "",
                        "wikiLink" : ""
                    }    
                }},
            {"id":4,"lng":-46.549072265625,"lat":31.5129958574547,

                "translated_data" : {
                    "en" : {
                        "name" : "Help farmer Eda",
                        "desc" : "",
                        "wikiLink" : ""
                    },
                    "fr" : {
                        "name" : "Aider le fermier Eda",
                        "desc" : "",
                        "wikiLink" : ""
                    }    
                }},
            {"id":5,"lng":-37.430419921875,"lat":28.832746799225905,
                "translated_data" : {
                    "en" : {
                        "name" : "Assist the Seraph at Shaemoor Garrison",
                        "desc" : "",
                        "wikiLink" : ""
                    },
                    "fr" : {
                        "name" : "Assister les Séraphins à la garnison de Shaemoor",
                        "desc" : "",
                        "wikiLink" : ""
                    }    
                }}
        ]
        }
    ]
}
}

Note that there is no mention of the Area in the JS since it is computed server-side but it could be in the javascript config file.

lpdumas commented 12 years ago

cool! i'm on it ;)

jsilvestre commented 12 years ago

And I forgot:

 "name":"Explore",

Should be:

"translated_data" : {
    "en" : { "name" : "Explore" },
    "fr" :  { "name" : "Exploration" }
}

!

edit: here is a config.js properly generated by the ConfigGenerator component: https://gist.github.com/3093811

lpdumas commented 12 years ago

Hey! I've made some test with the new config structure, and we have a huge performance drop. Looks like the translated_data extra level is responsible.

jsilvestre commented 12 years ago

How is that possible oO GW2C doesn't loop on it so it shouldn't be an issue :-/ Also, this structure should avoid too much loop/recursion too. Can't it be the addition of new markers ? Do you have any idea of a way to benchmark it ? Can you push the code on a special branch ? (edit: found it).

We need to find the bottleneck before making conclusion/looking for solutions.

lpdumas commented 12 years ago

I'm gonna push on dev branch ( buggy ). It's very strange, markers are appearing something like 5 second after the map is loaded. I don't get it.

EDIT

I'm pretty sure the problem come form when adding new marker. Javascript is looping quickly through or data, but their is some latency from when the method addMarker is called, and when the marker is shown on the map. That's very strange because nothing has change when i come to marker creation, only some change like:

// new
marker["title"] = markerInfo["data_translation"][window.LANG]["title"]

// old
marker["title"] = markerInfo["title"]

jsilvestre commented 12 years ago

The problem comes from adding new marker, I agree (tested).

But this doesn't come from the new format I think.

        animation: isNew != null ? google.maps.Animation.DROP : false

There is an animation played for new marker (it is a bug, isNew is false and not null at that moment) and I think the latency comes from here. Comment it out (or even better fix the condition to isNew != false) you will see a difference.

Also, I have found some potential improvements. Sorry this is Javascript not CoffeeScript but I don't have much time to correct and send you a PR today :-/

      image = new google.maps.MarkerImage(iconPath, null, null, new google.maps.Point(iconmid, iconmid), new google.maps.Size(iconsize, iconsize));

We should have one instance per MarkerImage.

if(images[markersType] == null) {
        images[markersType] = new google.maps.MarkerImage(iconPath, null, null, new google.maps.Point(iconmid, iconmid), new google.maps.Size(iconsize, iconsize));
}
....
        icon: images[markersType], // in marker creation

Where images is a global variable.

Also, at the end of the function:

      _ref = this.gMarker[markersCat]["marker_types"];
      _results = [];

      for (_j = 0, _len1 = _ref.length; _j < _len1; _j++) {
        markerType = _ref[_j];
        if (markerType.slug === markersType) {
          _results.push(markerType["markers"].push(marker));
        }
      }
      return _results;

Can just be:

      this.gMarker[markersCat]["marker_types"][markersType]["markers"].push(marker);

(edit: after a test, this doesn't seem to work, I'm looking at it right now). (edit 2: ok I see why, we should use the marker type's slug as key to avoid this loop. This would mean replacing the array by an object. I think this would be more logical actually. What do you think ?)

I think we can also create the infoWindow when the user clicks on the marker instead of creating ALL the infoWindow at the beginning because they are not all useful to the user at the time of loading. We could imagine the same thing for hidden markers but that's more complex to achieve without a performance drop when toggling markers.

lpdumas commented 12 years ago

Cool! that worked :D the condition to checkt if the marker was a new one wasn't working properly ;)

As for the MarkerImage , I agree that 1 instance per type is require, i'm gonna work on this tomorrow.

Also, you're solution for pushing the marker in the object won't work because the gMarker structure is exactly like our config file structure, thereby it doesn't have a [markersType] key.

Finally, infowindows are already created on marker clicks ;)

jsilvestre commented 12 years ago

I didn't know for infoWindow, sorry !

Also, for my last paragraph you might have missed my edits :

(edit: after a test, this doesn't seem to work, I'm looking at it right now). (edit 2: ok I see why, we should use the marker type's slug as key to avoid this loop. This would mean replacing the array by an object. I think this would be more logical actually. What do you think ?)

lpdumas commented 12 years ago

I'll look into it tomorrow ;) I think like you that this would be better. Config file may change a bit . On Jul 12, 2012 10:15 AM, "Joseph Silvestre" < reply@reply.github.com> wrote:

I didn't know for infoWindow, sorry !

Also, for my last paragraph you might have missed my edits :

(edit: after a test, this doesn't seem to work, I'm looking at it right now). (edit 2: ok I see why, we should use the marker type's slug as key to avoid this loop. This would mean replacing the array by an object. I think this would be more logical actually. What do you think ?)

Reply to this email directly or view it on GitHub: https://github.com/lpdumas/gw2cartographers/issues/36#issuecomment-6936160

lpdumas commented 12 years ago

Hey! i worked on this and everything is fine now :D

config.js : https://github.com/lpdumas/gw2cartographers/blob/master/assets/javascripts/config.js (commented stuff is for me, i'm gonna add all trainers and pets)

Things to know:

marker_types is an object now, and not an array, each key represent a marker type ( example )
Default marker desc/title/wikilink obligatory means that theirs no data_translation for individual marker from this group. This mean we can't edit the content of a marker which have a default value (see line #740 to see what i mean by default value)
I've took your advice for marker image and only instanciate them 1 time (1 for each marker type) and store them in a global object

jsilvestre commented 12 years ago

This business is getting better and better ! Here are my remarks.

is the "slug" field still useful for marker_type sections since the key is the slug (like we have for marker_group sections) ?
I am not sure to fully understand your second point but when I look at the file we seem to agree on the way it should be :)

The good news is that the ConfigGenerator component of GW2C-Backend generates almost this format already. I am currently working on the edition of the default values and the translatable data in the admin to make it easy for us to modify everything. Currently this part is the only one that is not crowdsourced. That means we don't have to put those data in the exported JSON string.

I'm focusing on cleaning the code and making the backend functional again (this was actually working!! [with many buys ;-)]) so we can hopefully test it during BWE3 ! There will not be an advanced UI, I will probably need your help for that :-)

Do not add too many marker types because we will have to put in the database after the release.

One more thing, about the "version" metadata. This is really important we send it in the exported JSON string because the server must know against which revision of the map the modification has been made. This version is currently the database's revision ID but we can add both a "version" field (filled with the backend version) and a "map_revision" field (with the map revision) !

lpdumas commented 12 years ago

Happy to see that everything is getting together :D

On what your asking :

Slug field need to go ;)
I wont add to many marker ^^
I will put the version in the exported data

For the beta weekend, the backend UI is not very important, we'll focus on functionalities. Awesome work ^^!

lpdumas / gw2cartographers

Proper config.js format #36