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

Better cities and towns labels in 3D view #635

Merged
merged 18 commits into from
Jul 17, 2021
Merged

Better cities and towns labels in 3D view #635

merged 18 commits into from
Jul 17, 2021

Conversation

Rayzeq
Copy link
Contributor

@Rayzeq Rayzeq commented Jun 21, 2021

I also plan to do the state labels, but I have some difficulties so it might take more time

@Azgaar Azgaar self-requested a review June 21, 2021 17:06
@Azgaar
Copy link
Owner

Azgaar commented Jun 21, 2021

That's kinda cool, I love it! Will share on Discord so that people can give a feedback.

A couple of suggestions:

  • Auto-positioning is interesting, but a bit annoying and unpredictable. Can you made always them face observer?
  • Some labels are too low (under surface), capitals are probably to high
  • Is it possible to make labels follow the selected style? Like font, size etc.
  • Can you make it optional? We have 3d more settings dialog where you can add a checkbox.

@evolvedexperiment, please review as well if you have time.

@Azgaar
Copy link
Owner

Azgaar commented Jun 21, 2021

Burg.labels.3d.mp4

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jun 21, 2021

I'm glad you like it!

  • What do you mean by auto-positioning ? The labels are actually facing the camera, I guess you want the labels to be parallel to the camera as if it was a plane, just like this: (tell me which one you prefer between the old one and this one)
Capture.d.ecran.video.de.21-06-2021.21_08_52.mp4
  • For the labels that are a bit low and that go through the floor I can unfortunately do nothing but put them all higher. On the other hand I can without problems put lower the labels of the capitals
  • For the font it's not a problem, but for the size I don't know how to do it: the sizes on the svg are 3 and 7 and the sizes I use for the 3d are 7 and 25, if you have an idea to solve this problem I'm interested (I tried multiply by 2.7 but I don't like it). (I will add the commit when this size problem is solved)
  • No problem, I will see to do it and I will add the commit

PS: I'm French so there's probably some problem in my message, don't mind it

@Azgaar
Copy link
Owner

Azgaar commented Jun 21, 2021

It looks much better. The labels were changing position too unpredictable, as least for me. For the elevation - no need to extract burg data from svg, all the data is available as pack.burgs. Here you can get coordinates, status (capital or not), cell number and so on. Elevation is in pack.cells.h typed array (burg cell is index).

As for size - it requires testing, maybe the current variant is the best.

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jun 21, 2021

I already get the elevation from pack.cells.h, I think the problem is that Three.js interpolates the height between the points, while pack.cells.h uses an average of the points or just one of the points, I'm not sure. Anyway I agree that I should use pack.burgs instead of the svg.

For the size I think the actual values are good, the only problem is that it's hardcoded.

@evolvedexperiment
Copy link
Contributor

It looks great. I didn't find any bugs. The only thing I'd suggest is to use the label colours from the style.

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jun 24, 2021

Well, as the pull request has not been merged yet, I take the opportunity to add the state labels. By the way, I totally replaced the canvas by an svg (because I need textpaths). And now the labels toggle doesn't do a full reload anymore (but causes a huge freeze).

@Azgaar
Copy link
Owner

Azgaar commented Jun 28, 2021

Hello, will you continue to work on it?

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jun 29, 2021

No, I don't see anything more to add, you can merge the pull request.

modules/ui/3d.js Outdated Show resolved Hide resolved
modules/save.js Outdated Show resolved Hide resolved
modules/ui/3d.js Outdated Show resolved Hide resolved
modules/ui/3d.js Outdated Show resolved Hide resolved
@Azgaar
Copy link
Owner

Azgaar commented Jun 29, 2021

Hello, please check the comment above. I believe some cleansing required before merging.

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jun 30, 2021

Everything is fixed now

@Azgaar
Copy link
Owner

Azgaar commented Jul 1, 2021

thanks for the update, it looks good. Just a few issues:

  • Burg labels are blurry for some reason
  • Only default font is loading
  • Can state labels be vertical like burg ones?

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jul 1, 2021

I fixed some issues but I' ll wait your answer before making a commit:

  • I increased the resolution of the labels, I didn't want to put it too high to use less memory but it looks ok.
  • You probably didn't use the latest commit, which works without any problem for me
  • I can do it but I think it would be hard to know which label belongs to which state (probably badly worded).
  • I also fixed a big memory leak, so that deactivating and reactivating labels does not use more and more memory.

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jul 1, 2021

I tested several fonts and I just realized that some of them do not work, I will investigate this tomorrow

@Azgaar
Copy link
Owner

Azgaar commented Jul 1, 2021

thanks!

@Azgaar
Copy link
Owner

Azgaar commented Jul 3, 2021

Thanks, I will try to review the PR next week.

@Azgaar
Copy link
Owner

Azgaar commented Jul 5, 2021

Hello, I'm ok to merge, but still believe there is room for improvement.

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jul 5, 2021

  • The raycasting works much better! On the other hand, it makes the change of the height scale very laggy, but it is not very important, it is always possible to deactivate the labels to change the height scale and reactivate them afterwards
  • yeah I agree, it uses too much cpu but I don't know how to optimize it more (except using InstancedMesh for the icons, but Three.js is too old, I'll try to update it). The problem with using the same mesh for all labels is that it will look something like this:
Capture.d.ecran.video.de.05-07-2021.18_16_54.mp4
  • Using TextGeometry would probably be even worse for performance, and it would require including all the fonts as .json files, so I think it wouldn't be a good idea
  • btw I noticed that the first element of pack.burgs is just {name: undefined}, is this a bug?

@Azgaar
Copy link
Owner

Azgaar commented Jul 5, 2021

It would be nice if all labels can be rendered from the same texture, but yes, I'm not sure whether it's possible. But creating a separate svg with font embedded for each label, and then turning it to canvas, that's definitely not the best way to handle it.

Burg[0] is a placeholder showing that element 0 is "no burg". I keep it for historical compatibility issues. It would be much faster to just skip first element than slice the whole array.

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jul 6, 2021

Rendering all labels on the same texture and tweaking the mesh UVs is probably possible, but it would result in overlapping labels as they are scaled up for better quality. But the current solution has no impact on performance, except during the initial freeze, as the svg are turned into normal textures by the TextureLoader.
About the slice, I understand that it should not copy the array, MDN says The slice() method returns a shallow copy, but maybe I don't understand correctly, I am more used to python.I also read that the for loop is faster than for..of so I replaced the old loop which solves the problem.

@Azgaar
Copy link
Owner

Azgaar commented Jul 6, 2021

I believe it depends on you PC, for me it's acceptable, but close to freeze. So UX is kinda bad. I would really like to get it better. The current labels quality is still pretty low, so I believe so other approach van be tested. But that's up to you. The only real difference is that if it runs faster, we can turn the option on de default.

As for slice() - it copies the array, losing links, so basically provides a copy of you array if you don't have advanced stuff inside. For loops should be both optimized, browsers had some issues with them, but now the consensus is that js engines can handle all them in an optimal way.

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jul 6, 2021

So, I reverted to using a canvas for the town labels, which makes the loading much faster (the state labels don't use a canvas because curved text is no possible with them). I also increased the quality of both types of labels, but it may not be enough for large monitors (should I create an option to change it manually ?).

@Azgaar
Copy link
Owner

Azgaar commented Jul 10, 2021

I believe it's fine, need to find some time for a final review and then merge

@Azgaar Azgaar merged commit 4575edc into Azgaar:master Jul 17, 2021
@Azgaar
Copy link
Owner

Azgaar commented Jul 17, 2021

Merged, thanks for the contribution!

@Azgaar
Copy link
Owner

Azgaar commented Jul 17, 2021

I have played with it a bit and the performance happen to be very bad. I have added some optimizations and finally moved from canvas text to sprites Sprites have better quality and we don't need to change their position as sprites always face user).

I've also change icon style to be more interesting and fixed a couple of bugs.

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jul 18, 2021

Wow really better performance, I didn't know about Sprites. I found some possible improvements:

  • In textureToSprite:
    • Set sprite.renderOrder to 1 to solve depth issues (black background of labels + labels disappear for no reason when their orientation changes), and you can remove material.depthTest = false
    • Set map.anisotropy to Renderer.getMaxAnisotropy() to improve the quality of the mipmaps (it makes a huge difference)
  • Add doWorkOnRender(); to the end of createLabels because the labels don't hide if you don't move otherwise

You can check these changes here : Rayzeq/Fantasy-Map-Generator , I didn't make a pull request because there are only 4 lines to edit.

@Azgaar
Copy link
Owner

Azgaar commented Jul 18, 2021

Cool, I've added these improvements as well. I've also think about rendering state borders as 3d path, as texture resolution is too low. Actually, we can use SVGRenderer instead of raster, but it would be really hard to make huge svg to work fast. So we can make 3d just some elements that work bad as texture (like borders, markers and so on).

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jul 18, 2021

Yes, this is the main reason I decided to separate the labels from the landscape. I think we could use lines and small 3D models (like town icons).
By the way, I just discovered that Renderer.getMaxAnisotropy() is deprecated in favor of Renderer.capabilities.getMaxAnisotropy().

@Azgaar
Copy link
Owner

Azgaar commented Jul 18, 2021

Ok, let's see how it goes

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jul 19, 2021

I've done state borders using lines (Rayzeq/Fantasy-Map-Generator it's just a proof of concept, absolutely not clean and debugged) but I don't know if that's a good idea, as lines in OpenGL/WebGL are not actively supported and unofficially deprecated (or not designed to be actually used).

@Azgaar
Copy link
Owner

Azgaar commented Jul 20, 2021

That's interesting, but I wasn't able to check it life, how is it supposed to work?

@Rayzeq
Copy link
Contributor Author

Rayzeq commented Jul 20, 2021

Normally you just go to the 3d view to see the lines, but your webgl implementation may not support lines at all. I didn't think that lines were so badly supported, it is probably necessary to use meshes.

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

Successfully merging this pull request may close these issues.

3 participants