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

Indoor mode #3097

Closed
wants to merge 39 commits into
from

Conversation

5 participants
@zbycz
Contributor

zbycz commented May 3, 2016

Hi,

I am submitting a little larger pull request. I am not sure if its possible to be merged as is, but I did my best and will be happy to cooperate further to make it so.

I’ve been engaged in OpenStreetMap since 2008 and for the time being I’ve led many workshops, written some school works and worked on getting the Czech community together on openstreetmap.cz. Last summer I had to come up with a topic for my final engineer thesis, so I decided to use the effort already invested in indoor mapping and chose to update the iD editor accordingly. Let me elaborate...

Current work-in-progress proposal is called Simple Indoor Tagging. I didn’t want to rely on WIP, so I realized there is already an accepted solution – the level=* tag.

There are two challenges for indoor:

  1. making underlying background map (this is addressed by Simple Indoor’s indoor=* area features)
  2. display ways – the accessibility of places – and this kind of was the nature of OSM from the beginning. Until we had all the landuse=* areas, OSM started with highways + pois.

Drawing highways=* for indoor feels very natural – corridors usually repeats every floor even in multi-storey buildings, it displays clear accessibility information, can be already used for routing and also works with parking highways. The logic is also very simple - when a level=* feature is in viewport - show “Indoor” button, and let the user filter on these.

Several examples (my build):

Well, i think I made my point - let’s discuss the technical background :-)


Some thoughts / questions:

  1. Is it ok, to call it indoor mode? It doesn't fit with current “modes” terminology, where modes are exclusive.
  2. I think the philosophy of iD is to show disabled buttons for “availible” actions. I chose to hide the button completely, where there isn't any level=* features on screen. Would you agree?
  3. Current level=* allows only separate values by semicolon, I borrowed three useful tagging from SimpleIndoor. Is it ok to use these? Should i maybe make new official proposition?
    • ranges – level=0-12 or -3--1 for features spaning several floors.
    • repeat_on – for repeating the same (possibly multilevel) feature on other floors - ie. doors,toilets
    • min/max_level – when set on building, the building spans through these levels. It helps as a visual cue, and also for the renderers.
  4. I had to unclip the building-outline shape, because a building completely covered with indoor=* areas, couldn't be clicked anyway. Kind of addresses the issue #2225.
  5. I submitted my original development commits, rebased to current master. Is it appropriate to squash the commits into one or few?
  6. Although I tried to abide the current code style and wrote some test, tell me if i should change something globally. Easiest would be to comment the code in the specific commits..

Something to do in future

  • if agreed, I would like to add level indicator as shown here https://github.com/zbycz/iD/wiki/devnotes-zbycz – but rather use another svg layer instead of filters.
  • once proposition is accepted – enable switching over floating point level values (ie. level=1.5). But even now - any string value could be written in the level chooser, so it “works”.
  • add icon for ‘highway=elevatorandindoor=corridor+stairs=yes` as these had to be distinguished easily.
  • Unfortunately I found later, I developed very similar changes as Panier’s id-indoor fork. Although he added many more indoor presets, which could be eventually merged here.
  • Create a level switcher in osmbuildings.org 3D viewer ;-) .. well, distant future indeed.

Changelog until now:

  • added iD.ui.IndoorMode control ui
  • added filtering using features.indoor_different_levels
  • added .indoor-mode class for svg surface
  • added general preset for indoor=* when indoor-mode enabled
  • added several map styles for enabled .indoor-mode
  • added indoor_levels as general field for building, using iD.ui.preset.range

btw, it was a pleasure to work with iD codebase, also d3 is so very amazing :-)

zbycz added some commits Mar 26, 2016

@bhousel

This comment has been minimized.

Show comment
Hide comment
@bhousel

bhousel May 3, 2016

Member

Wow, impressive work @zbycz! I really like this and would like to see this merged in some form.

I'll need some time to think about how to present the UI in a way that doesn't clutter things up too much.

I'll try to answer your questions now:

Is it ok, to call it indoor mode? It doesn't fit with current “modes” terminology, where modes are exclusive.

I think it's ok to call it indoor mode. (People will call it that anyway). You're right it's not technically a "mode" like the other exclusive drawing modes.

I think the philosophy of iD is to show disabled buttons for “available” actions. I chose to hide the button completely, where there isn't any level=* features on screen. Would you agree?

I'm leaning towards having the button smaller but available, maybe just an icon with tooltip, and it can transition open to reveal the indoor drawing controls when active. (I'm struggling with this same problem on #2699 - it's cool and useful, but where to put it?)

Current level=* allows only separate values by semicolon, I borrowed three useful tagging from SimpleIndoor. Is it ok to use these? Should i maybe make new official proposition?
rangeslevel=0-12 or -3--1 for features spaning several floors.
repeat_on – for repeating the same (possibly multilevel) feature on other floors - ie. doors,toilets
min/max_level – when set on building, the building spans through these levels. It helps as a visual cue, and also for the renderers.

The right thing to do is to make a proposal and loop in the OSM tagging list. At this point, indoor editing is not solidly established, so I think you have a good chance of pushing the community in a sane direction.

I had to unclip the building-outline shape, because a building completely covered with indoor=* areas, couldn't be clicked anyway. Kind of addresses the issue #2225.

Makes sense.. Yes I agree that in "indoor mode" the building outline should not render as an area, as this covers up the indoor features.

I submitted my original development commits, rebased to current master. Is it appropriate to squash the commits into one or few?

It would be better to squash them into fewer commits, but I'm not super picky about this.

Although I tried to abide the current code style and wrote some test, tell me if i should change something globally. Easiest would be to comment the code in the specific commits..

I'll try to review it in more detail, but what I saw at a quick glance looks great! And the tests are much appreciated...

Member

bhousel commented May 3, 2016

Wow, impressive work @zbycz! I really like this and would like to see this merged in some form.

I'll need some time to think about how to present the UI in a way that doesn't clutter things up too much.

I'll try to answer your questions now:

Is it ok, to call it indoor mode? It doesn't fit with current “modes” terminology, where modes are exclusive.

I think it's ok to call it indoor mode. (People will call it that anyway). You're right it's not technically a "mode" like the other exclusive drawing modes.

I think the philosophy of iD is to show disabled buttons for “available” actions. I chose to hide the button completely, where there isn't any level=* features on screen. Would you agree?

I'm leaning towards having the button smaller but available, maybe just an icon with tooltip, and it can transition open to reveal the indoor drawing controls when active. (I'm struggling with this same problem on #2699 - it's cool and useful, but where to put it?)

Current level=* allows only separate values by semicolon, I borrowed three useful tagging from SimpleIndoor. Is it ok to use these? Should i maybe make new official proposition?
rangeslevel=0-12 or -3--1 for features spaning several floors.
repeat_on – for repeating the same (possibly multilevel) feature on other floors - ie. doors,toilets
min/max_level – when set on building, the building spans through these levels. It helps as a visual cue, and also for the renderers.

The right thing to do is to make a proposal and loop in the OSM tagging list. At this point, indoor editing is not solidly established, so I think you have a good chance of pushing the community in a sane direction.

I had to unclip the building-outline shape, because a building completely covered with indoor=* areas, couldn't be clicked anyway. Kind of addresses the issue #2225.

Makes sense.. Yes I agree that in "indoor mode" the building outline should not render as an area, as this covers up the indoor features.

I submitted my original development commits, rebased to current master. Is it appropriate to squash the commits into one or few?

It would be better to squash them into fewer commits, but I'm not super picky about this.

Although I tried to abide the current code style and wrote some test, tell me if i should change something globally. Easiest would be to comment the code in the specific commits..

I'll try to review it in more detail, but what I saw at a quick glance looks great! And the tests are much appreciated...

@zbycz

This comment has been minimized.

Show comment
Hide comment
@zbycz

zbycz May 5, 2016

Contributor

Thanks, @bhousel !

I will make the proposal next week, in the meantime I am looking forward to your code comments 👍 Once we're done, I will sqash the commits in fewer.

ad button positioning) the more buttons, the more the user think how complex the software is (aka Norman: The design of everyday things) - and I think iD should stay "simple". But also its a bad practise for controls to disappear unexpectedly.

I completely agree with a smaller icon expanding to full control. What about placing Indoor-mode button in a dropdown menu "Tools" on the right of the top toolbar. It could be left closed forever, or left toggled opened. It would be a nice drawer for more useful tools in the future (including the #2699).

Contributor

zbycz commented May 5, 2016

Thanks, @bhousel !

I will make the proposal next week, in the meantime I am looking forward to your code comments 👍 Once we're done, I will sqash the commits in fewer.

ad button positioning) the more buttons, the more the user think how complex the software is (aka Norman: The design of everyday things) - and I think iD should stay "simple". But also its a bad practise for controls to disappear unexpectedly.

I completely agree with a smaller icon expanding to full control. What about placing Indoor-mode button in a dropdown menu "Tools" on the right of the top toolbar. It could be left closed forever, or left toggled opened. It would be a nice drawer for more useful tools in the future (including the #2699).

@bhousel

This comment has been minimized.

Show comment
Hide comment
@bhousel

bhousel May 5, 2016

Member

What about placing Indoor-mode button in a dropdown menu "Tools" on the right of the top toolbar. It could be left closed forever, or left toggled opened. It would be a nice drawer for more useful tools in the future

I've been playing around with making the UI more responsive and supporting something like a "drawer" for more commands or options on existing commands.

I don't really like it.. Maybe @samanpwbb has ideas?

The problem is it either bumps down the other things in the layout, or it overlaps them. So we'd either need to 1. position the other UI stuff differently or 2. be ok about having them move (eh) or 3. something else.

responsive_bar

Member

bhousel commented May 5, 2016

What about placing Indoor-mode button in a dropdown menu "Tools" on the right of the top toolbar. It could be left closed forever, or left toggled opened. It would be a nice drawer for more useful tools in the future

I've been playing around with making the UI more responsive and supporting something like a "drawer" for more commands or options on existing commands.

I don't really like it.. Maybe @samanpwbb has ideas?

The problem is it either bumps down the other things in the layout, or it overlaps them. So we'd either need to 1. position the other UI stuff differently or 2. be ok about having them move (eh) or 3. something else.

responsive_bar

@@ -1048,6 +1071,9 @@ button.save.has-count .count::before {
border-radius: 0 0 4px 4px;
overflow: hidden;
}
.form-field > input:nth-child(3) {
border-left: 0;
}

This comment has been minimized.

@zbycz

zbycz May 7, 2016

Contributor

Because the ui.type.range needs second input without border.
Would you suggest a better selector maybe?

@zbycz

zbycz May 7, 2016

Contributor

Because the ui.type.range needs second input without border.
Would you suggest a better selector maybe?

.indoor-mode text.arealabel-halo.tag-building,
.indoor-mode text.arealabel-halo.tag-amenity {
display: none;
}

This comment has been minimized.

@zbycz

zbycz May 7, 2016

Contributor

I`m not sure this is the right way to hide the building icon+label. It possibly breaks if other tags get precedence over building=* or amenity=* and shows its icon. Then it wouldnt be hidden.

Can you think of a better way, please? Or do you think its fine this way?

@zbycz

zbycz May 7, 2016

Contributor

I`m not sure this is the right way to hide the building icon+label. It possibly breaks if other tags get precedence over building=* or amenity=* and shows its icon. Then it wouldnt be hidden.

Can you think of a better way, please? Or do you think its fine this way?

tags.level = context.indoor().level();
}
return tags;
}

This comment has been minimized.

@zbycz

zbycz May 7, 2016

Contributor

I dont particulary like this code, b/c it repeats in all add_* files. Still i dont think it should use other dependency.. So maybe its fine.

@zbycz

zbycz May 7, 2016

Contributor

I dont particulary like this code, b/c it repeats in all add_* files. Still i dont think it should use other dependency.. So maybe its fine.

return true;
}
});

This comment has been minimized.

@zbycz

zbycz May 7, 2016

Contributor

Now it shows all the time in the "map features" panel. But maybe it doesnt make sense to show this particular feature. It could be added/removed when indoor is toggled, but i like it this way better. (Also something is using it lower down in this file..)

Conclusion - maybe just hide the checkbox when indoor mode is not active?

@zbycz

zbycz May 7, 2016

Contributor

Now it shows all the time in the "map features" panel. But maybe it doesnt make sense to show this particular feature. It could be added/removed when indoor is toggled, but i like it this way better. (Also something is using it lower down in this file..)

Conclusion - maybe just hide the checkbox when indoor mode is not active?

if ((!context.indoor().enabled() && entity.isNew()) || geometry === 'point') return false;
//hide vertices with different level
if (geometry === 'vertex' && context.indoor().enabled() && _features.indoor_different_level.filter(entity, resolver, geometry)) return true;

This comment has been minimized.

@zbycz

zbycz May 7, 2016

Contributor

Default behaviour is iD shows new entities regardless the feature filter. But here i need to hide different level always..

Also for the code clarity i replaced !entity.version for entity.isNew() – is it correct?

@zbycz

zbycz May 7, 2016

Contributor

Default behaviour is iD shows new entities regardless the feature filter. But here i need to hide different level always..

Also for the code clarity i replaced !entity.version for entity.isNew() – is it correct?

if (!context.surface()) { // hash calls before surface is initialized
setTimeout(indoor.toggle, 200);
return false;
}

This comment has been minimized.

@zbycz

zbycz May 7, 2016

Contributor

I dont like this solution, but havent found better..

@zbycz

zbycz May 7, 2016

Contributor

I dont like this solution, but havent found better..

@PanierAvide

This comment has been minimized.

Show comment
Hide comment
@PanierAvide

PanierAvide May 7, 2016

I'm glad to see the opportunity of integrating indoor editing in main iD version. Feel free to reuse any custom part of iD-indoor, like presets or level parser (which handles lot of level-related tags) ;)

I'm glad to see the opportunity of integrating indoor editing in main iD version. Feel free to reuse any custom part of iD-indoor, like presets or level parser (which handles lot of level-related tags) ;)

@samanpwbb

This comment has been minimized.

Show comment
Hide comment
@samanpwbb

samanpwbb May 10, 2016

Member

Indoor mode would work well as a Map data filter:

screen shot 2016-05-10 at 2 40 40 pm

If the auto-tagging piece of this PR is mandatory, we can add a note about it in the filter panel:
screen shot 2016-05-10 at 2 46 06 pm

Member

samanpwbb commented May 10, 2016

Indoor mode would work well as a Map data filter:

screen shot 2016-05-10 at 2 40 40 pm

If the auto-tagging piece of this PR is mandatory, we can add a note about it in the filter panel:
screen shot 2016-05-10 at 2 46 06 pm

@bhousel bhousel referenced this pull request May 19, 2016

Open

iD Taskbar #3123

if (context.indoor().enabled())
q.level = context.indoor().level();
else
delete q.level;

This comment has been minimized.

@jirutka

jirutka Jun 13, 2016

@zbycz Please never ever omit curly braces around if-else, it’s generally considered as a bad practice and there are many documented cases when it caused a security issue.

@jirutka

jirutka Jun 13, 2016

@zbycz Please never ever omit curly braces around if-else, it’s generally considered as a bad practice and there are many documented cases when it caused a security issue.

enterButton = selection.append('button')
.attr('class', 'indoormode-enter-button col12 ')
.on('click', toggleIndoor)
.call(buttonTooltip(t('indoor_mode.help'), '⌘⇧I'));

This comment has been minimized.

@jirutka

jirutka Jun 13, 2016

I think that this shortcut will not work on other platforms than OS X…

@jirutka

jirutka Jun 13, 2016

I think that this shortcut will not work on other platforms than OS X…

// ---- tagging related -----
var rangeRegExp = /^(-?\d+(?:\.\d+)?)(?:(-)(-?\d+(?:\.\d+)?)|(;-?\d(?:\.\d+)?)+)?$/;

This comment has been minimized.

@jirutka

jirutka Jun 13, 2016

It’d be helpful to add some examples of inputs that this regexp parses…

@jirutka

jirutka Jun 13, 2016

It’d be helpful to add some examples of inputs that this regexp parses…

@zbycz

This comment has been minimized.

Show comment
Hide comment
@zbycz

zbycz Feb 8, 2017

Contributor

Hi, just letting you know, we have moved a little further. I created official proposal and submited RFC to tagging list.

In near future i will create new pull-request based on current master, thus I am closing this one.

Contributor

zbycz commented Feb 8, 2017

Hi, just letting you know, we have moved a little further. I created official proposal and submited RFC to tagging list.

In near future i will create new pull-request based on current master, thus I am closing this one.

@zbycz zbycz closed this Feb 8, 2017

@bhousel

This comment has been minimized.

Show comment
Hide comment
@bhousel

bhousel Feb 8, 2017

Member

Hi, just letting you know, we have moved a little further. I created official proposal and submited RFC to tagging list.

In near future i will create new pull-request based on current master, thus I am closing this one.

Great! We are thinking a lot about plugins lately and I would love to find a good way to merge your work into iD to make indoor editing easy for everyone.

Member

bhousel commented Feb 8, 2017

Hi, just letting you know, we have moved a little further. I created official proposal and submited RFC to tagging list.

In near future i will create new pull-request based on current master, thus I am closing this one.

Great! We are thinking a lot about plugins lately and I would love to find a good way to merge your work into iD to make indoor editing easy for everyone.

@bhousel bhousel added this to Toolbar in Priorities Jun 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment