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

Floating roads. #29

Open
johnsnook opened this issue Sep 11, 2016 · 12 comments
Open

Floating roads. #29

johnsnook opened this issue Sep 11, 2016 · 12 comments
Assignees
Labels

Comments

@johnsnook
Copy link

johnsnook commented Sep 11, 2016

Roads floating 2 units above ground. I've identified and submitted a fix in patch-1 branch based off dev.

@brnkhy
Copy link
Owner

brnkhy commented Sep 13, 2016

Hey John!
I'll check that, thanks!

@johnsnook
Copy link
Author

johnsnook commented Sep 13, 2016

I just saw that you pushed dev to master. Have you thought of replacing the Factory.Order with something like YPosition? I've found that .01 works best for landuse and water and .02 for roads. Ignore my previous patch, I think there's a better way.

Let me know how I can be helpful. I've built web GISs using php/javascript so i'm familiar with the basic mechanics. I'm new to unity and c# however.

@brnkhy
Copy link
Owner

brnkhy commented Sep 13, 2016

well I believe the "order" thing is flat out wrong really. I did it for a quick fix before but the real solution lies in the sort_key property.
see; https://mapzen.com/documentation/vector-tiles/layers/#feature-ordering
I'm not sure how to implement that, had a brief look but didn't have time to give it a shot. so yeah, it would be great if you can try it out!
Don't worry about the unity/c# stuff. If you can sort out the logic (cant use those numbers straight away I guess), I'll go over your implementation ;)

@johnsnook
Copy link
Author

johnsnook commented Sep 13, 2016

Correct me if I'm wrong, but I think sort key is really only applicable to 2d rendering (like HTML divs).

Since the environment is 3d, wouldn't it be better to simply expose a public Y for that layer? For example roads can be Y (.02) with water and landuse at Y(.01) without conflict and buildings should be zero. It's not like divs where everything is flat.

Unrelated: could make DynamicTileManger.Update "Public virtual" so i can override in my subclass?

@brnkhy brnkhy self-assigned this Sep 13, 2016
@brnkhy
Copy link
Owner

brnkhy commented Sep 13, 2016

Doesn't that sort_key correspond to the Y axis though? I thought they do and we can use them instead of testing random numbers like 0.1, 0.2. I might be totally wrong though, maybe I misunderstood it?

@johnsnook
Copy link
Author

johnsnook commented Sep 13, 2016

Doesn't that sort_key correspond to the Y axis though

Yes, but in a way that suggests layering one over the other in a very 2d way. By changing it to a y position (or some other, better variable name), it can be easily adjusted in the editor. .01 and .02 were values that I found to work well for me but others might want to do different things.

Sort key is the wrong paradigm because it suggests laying transparent layers (map then road then water, etc) on top of each other when they might be on the same Y as each other. water and landuse can be on the same Y because they never overlap. roads have to be higher than landuse because the DO sometimes overlap. Buildings can just be at Y = 0. Y is a practical value and shouldn't be tied to some other arbitrary value like sort. I mean, what are we sorting?

@brnkhy
Copy link
Owner

brnkhy commented Sep 13, 2016

hmm well you probably know GIS a lot more than me but what I meant was; why don't we use sort_key as reference for those float numbers?
Mapzen/openstreetmap data would be much more reliable than our quick assumptions like "landuse wont overlap with water". For example, if I recall correctly, water layer includes pools and they generally reside on parks (landuse).

@johnsnook
Copy link
Author

johnsnook commented Sep 14, 2016

This data is usually served up to a javascript frontend that has "layers" panel where the user can add layers or change in the index of the base layers. Sortkey facilitates where in the "z-index" those divs get rendered. But we're not constrained to 2 dimensions, so it's an inappropriate use basically.

You could re-use sortkey as a Y multiplier, but that's not what it's for, and it's misleading to anyone used to programming web GIS. It seems it be better and simpler to just give Factory a YPosition that you can set in the scene or just use the default for each different factory. Then if you only want to use one or two of the factories, you don't have to "re-index" the sortkey. I mean, you might as well just use the order in the hierarchy if you're going to use an arbitrary sequential "key".

That's just my two cents. If you really don't agree, it's no big deal, I can just do it my way in a subclass.

Also, in general would you mind making monobehavior methods public and virtual? I want to be able to control behaviors in child classes of the tilemanagers, etc.

Keep up the good work!

@brnkhy
Copy link
Owner

brnkhy commented Sep 14, 2016

Not that I disagree or anything, I'm just trying to understand as I'm not familiar with the field. Another issue here would be, moving away from being a pure Mapzen visualization. It wasn't our goal to start with but still, it's a nice feature.
Still it's cool I guess, let's go with Y parameters (order) path. I'm just a little unsure about how to decide those numbers and their maintenance (changing numbers every patch etc). But if you're convinced it'll be alright, let's try it :)

I'm a little strict about making stuff public. Component shouldn't really know about each other unless they really need to. Let me know what you need and I'll look into that, might be nothing or we might have to find an alternative way etc.

Thanks man!

@johnsnook
Copy link
Author

johnsnook commented Sep 14, 2016

Another thought: Order/Yposition should probably be a setting since you've already made a nice settings system.

I said public but I meant protected. Protected virtual is better, you're right, especially if it's just for inheritance purposes.

@johnsnook
Copy link
Author

johnsnook commented Sep 14, 2016

Ok, another question, related to this topic or maybe just a bug.

Why are you setting the map y position to -1 in the MapImagePlugin on line 24?

I changed it from
go.localPosition -= new Vector3(0, 1, 0);
to
go.localPosition -= new Vector3(0, 0, 0);

without ill effect.

Also, because I believe this got lost in the shuffle:
in RoadFactory on Lines 53 and 82 you have:
road.transform.position += Vector3.up * road.SortKey / 100;

and then you do it again in on 103, bumping it even higher:
go.transform.position += Vector3.up * Order;

I believe the proper (temp) fix is to insert
if (!MergeMeshes)
before line 53 and 82 to prevent them getting raised higher when you're already doing it in your CreateLayer function. Or just remove it from the CreateLayer function.

@brnkhy
Copy link
Owner

brnkhy commented Sep 14, 2016

admittedly, factories are quite a mess at the moment. you'll find some checks here, nothing there, some fix here, bug there. Everything I did about Y axis (all those above) was barely testing so don't hesitate to change/fix. I'll eventually tidy up the factories as well but I'm waiting for threading stuff (I'll have to do that as well, and it'll change data movement etc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants