alternative grid generators #832

goteguru · 2022-06-07T23:14:29Z

Description

Alternative grid generators using Voronoi's power.
Just a PoC addressing #829. IDK it is useful or not.

Choose the grid algo from options.

netlify · 2022-06-07T23:14:42Z

✅ Deploy Preview for afmg ready!

Name	Link
🔨 Latest commit	`f4b3662`
🔍 Latest deploy log	https://app.netlify.com/sites/afmg/deploys/62b0a2e3ea2b210008b1af5c
😎 Deploy Preview	https://deploy-preview-832--afmg.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Azgaar · 2022-06-08T07:26:57Z

Yes, it will be pretty helpful to have for some categories of users. That's actually pretty great. We had something like that before, but it was removed for some reason, probably just an experiment as well.

We can use it in production with some cleanup, I also see some weird bug that map is not getting scrolled on drag. As for grid variants, we can also add flat hex as people always request it:

jittered square grid (current, I believe 'voronoi' is not correct, as finally all are voronoi, we can use 'jittered' or ''default' on UI)
square grid
hex pointy
hex flat

tukkek · 2022-06-09T02:12:41Z

map is not getting scrolled on drag

Just wanted to report that drag is working fine for me (Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0) - and mention that this feature is great!

goteguru · 2022-06-09T10:39:27Z

Ok, let's add some cleanup then. I think removing (or making optional) coastline interpolation and sea-cell optimization would be also useful for some use cases (eg hex-grids).

We can use it in production with some cleanup, I also see some weird bug that map is not getting scrolled on drag. As for grid variants, we can also add flat hex as people always request it:
I believe 'voronoi' is not correct, as finally all are voronoi

It might be technically incorrect, but pretty much correct from the user's perspective. One is generic voronoi the other is a hexgrid happen to be generated using the same algo. To be honest, if we remove the cell optimizations for hexgrids, it will become technically correct because all (center) cells will be actually hexes the concrete generation method doesn't matter. The data structure is not bound to voronoi cells we could store penrose tiling or even star-tiling :).

However I agree on that the user might not know what "voronoi" is (same for "jittered grid"). How about "random" or "irregular"?

* hex flat

Okay. Probably it would be a nice touch to make the "grid" layer to match cell size by default if "hex" is selected.

Bonus: you can resample your existing maps to hexgrids! :-D

Azgaar · 2022-06-09T13:24:15Z

How about "random" or "irregular"?

I prefer just 'default'.

goteguru · 2022-06-18T22:25:38Z

Updates:

Flat hex grid
load / save support (new data field for option)
submap support (you can remap your maps using different grid type)

First I was trying to simply swap rows and cols for flat hexes, but it seems some of the algos (eg. wind generation and icebergs) are not generic and depend on a row based layout, which is unfortunate and caused weird bugs. I rewrote the flathex algo to be row-based. Reverse id mapping for grids is also implementation specific and had to be changed. Current solution is a bit hacky but works.

Azgaar · 2022-06-19T13:04:17Z

Is it work in progress or somehow final result? I see it can be refactored quite a lot

goteguru · 2022-06-19T18:38:41Z

Somewhat final. :) what would you like to be refractored? Actually the whole generator could be refactored and all new features are patches. I follow the least modification principle. But if you tell me what you don't like it can be done Azgaar ***@***.***> ezt írta (időpont: 2022. jún. 19., Vas 15:04):

…

Is it work in progress or somehow final result? I see it can be refactored quite a lot — Reply to this email directly, view it on GitHub <#832 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUMDVMOYK7ILOXE5QOCR4LVP4LFXANCNFSM5YESQU7Q> . You are receiving this because you were assigned.Message ID: ***@***.***>

Azgaar

For new code we need to stick with better js, that will allow to modernize the whole codebase eventually.

I have added some comments, but I can implement on my own, I was going to work on them in any case

Azgaar · 2022-06-19T20:06:31Z

index.html

@@ -1500,6 +1500,20 @@
              </td>
            </tr>

+            <tr data-tip="Select grid generation algorithm">
+              <td></td>
+              <td>Grid algorithm</td>


Algorithm is too technical, it can be just 'Grid type'

Azgaar · 2022-06-19T20:07:50Z

index.html

+              <td>Grid algorithm</td>
+              <td>
+                <select id="gridAlgorithm">
+                  <option value="voronoiPoints" selected>Default</option>


Here is the opposite, we should use inner names as they are, so it should be like 'jittered'

I don't consider it a huge impact change, but if you think it's better... so be it.

Azgaar · 2022-06-19T20:10:42Z

main.js

@@ -642,8 +643,10 @@ async function generate(options) {

    applyMapSize();
    randomizeOptions();
+    const method = window[document.getElementById('gridAlgorithm').value];


Why it's in window?

I see that function is linked, that's pretty hard to read

it must be window. Because we are using global functions here. (Yes, we shouldn't, but if we start to refactor everything, we'd better off with a completely new project :-P, I've drawn a line here). The key can be be swapped to byId() if you like. I don't think it's hard to read :-o pretty straightforward.

I believe we just don't need this reference at all. And even if we need, we can use map object (dict)

why to add a map if we already have it? (the object).
Maybe I just don't get it what you mean, I'm quite sleepy. :) Tomorrow evening I'll check out your suggestions.

if we need a reference to a function, it's better to create a map for it than reference window

hmmm... why would it be so? window is just an object. And functions are just objects (and methods are actually maps from String to Function). I agree on that we could use globalThis instead of window if we'd like to be super compatible, but it's irrelevant .

Of course in an ideal case, these generators shouldn't be globals in the first place, and we could index the module object. It feels pretty clear and natural (to me at least), introducing a map for what we already have is less readable and redundant. The only thing which probably a bit ugly is that we can access any functions not just the generators, but this is the problem of the monolith architecture (globals). The proper way would be to namespace the generators under some module and use references from there. However I didn't wanted to rewrite everything, just do a small patch :)

ok, I change it a bit...

We have functions and string options, but not the connection between them. So we will need something like map in any case, so it's better to create a map. Also it will work when functions are no longer be exposed to global scope

err... functions has a name. (see Function prototype: name). It's quite a connection. :) It would work in case of modules too. Let's see I'll do it, if it's not working we'll go for maps :-D

Azgaar · 2022-06-19T20:14:40Z

main.js

 function reGraph() {
  TIME && console.time("reGraph");
  const {cells: gridCells, points, features} = grid;
  const newCells = {p: [], g: [], h: []}; // store new data
  const spacing2 = grid.spacing ** 2;
+  const optimize = gridOptimizationRequired()


Here it can be a simple check:

const gridType = document.getElementById('gridType')?.value; const optimizeWaterCells = gridType === "jiterred";

that's not exactly the same, and not a good design. String constants of the module (which is non-existent here of course :) shouldn't be used outside the module. One always should export an accessor (like gridOptimizationRequired) or define constants. Using functions instead of those "magic strings" has an additional advantage: your IDE can identify misspelling.
Of course there is no module, therefore the whole question is academic, but this accessor is a reminder to do the proper thing when the stuff will finally become a module.
We can remove it if you don't like it, but that one has a purpose.

Here I agree, but in this case gridOptimizationRequired should not be in main.js. We can have a separate module for points generation and put this function there. Something like 'points-generator'

Of course it shouldn't.There are quite a lot of stuff what shouldn't be there. :) It started as a small patch not a full blown refactoring project. That's the other branch. ^-^

Azgaar · 2022-06-19T20:15:58Z

main.js


  for (const i of gridCells.i) {
    const height = gridCells.h[i];
    const type = gridCells.t[i];
-    if (height < 20 && type !== -1 && type !== -2) continue; // exclude all deep ocean points
-    if (type === -2 && (i % 4 === 0 || features[gridCells.f[i]].type === "lake")) continue; // exclude non-coastal lake points
+    if (optimize) {


If optimization is not required, probably the whole rePack is not required as well

sure. you are absolutely right. But that pack object is super coupled with er... everything. I didn't dare to eliminate it. This is the safe bet. When we rewrite this part, this can be changed. If you are absolutely sure there will be no problem (you know the code much better than me) let's change it.

Agree, but we already can do a minor refactoring, leaving the resulting pack object as is. We can extract the logic for points repacking into a separate function and call it conditionally. If optimization is not required, then just return existing p, g and h arrays (one day will have to rename these craziness 😀)

yes, that was my first intention, but I was lazy to do it. It would certainly save some bytes but memory is cheap even in case of 100k maps (SVG eats several orders of magnitude more ram).

It's more of a refactoring issue. Adding more options we make the code harder to understand, so if anything new is added, we need to make the old part simpler to not increase the entropy too much

Azgaar · 2022-06-19T20:19:13Z

modules/io/save.js

@@ -34,7 +34,8 @@ function getMapData() {
    +hideLabels.checked,
    stylePreset.value,
    +rescaleLabels.checked,
-    urbanDensity
+    urbanDensity,
+    byId('gridAlgorithm').value,


The value will be incorrect if user change gridAlgorithm, but doesn't generate new map. So gridAlgorithm should be stored as a part of 'grid' object

Correct! I'll fix.

Azgaar · 2022-06-19T20:28:15Z

utils/graphUtils.js

 }

 // place random points to calculate Voronoi diagram
-function placePoints() {
+function voronoiPoints() {


We can reuse this function, mostly it's the same for all 3 sampling methods, can just pass different points generators

goteguru · 2022-06-20T16:48:21Z

Patched some parts, but the proper solution would an ES6 module. (Maybe I do it if I have time). Where to put modules? There is a dynamic directory but probably this is for modules loaded dynamically. Grid generators are certainly always loaded but still might be the right place. IDK, So the question is: where to put (static) ES6 modules?

Azgaar · 2022-06-20T18:52:38Z

Patched some parts, but the proper solution would an ES6 module. (Maybe I do it if I have time). Where to put modules? There is a dynamic directory but probably this is for modules loaded dynamically. Grid generators are certainly always loaded but still might be the right place. IDK, So the question is: where to put (static) ES6 modules?

We can move all 'generators' into a specific folder and create an 'esm' subfolder. But as we don't use bundling yet we will probably load this module via the script tag from index.html. So it can be in the same folder as other generators. Any solution will work, I don't have strong opinion here

goteguru · 2022-06-20T19:28:23Z

We can move all 'generators' into a specific folder and create an 'esm' subfolder. But as we don't use bundling yet we will probably load this module via the script tag from index.html. So it can be in the same folder as other generators. Any solution will work, I don't have strong opinion here

I don't think there will be that many point-field-generators. I would draw a border of abstraction at Grid level. Grids could be defined as a tuple of map and inverse map from ids to 2D coordinates Tuple<i32 => [float,float], [float,float] => i32>. Using this abstraction we could have finite, infinite maps, transformed (projected) maps. If we add "scale" to the inputs, we could have even recursive maps (kinda level of detail maps). But one step one time.

I just wanted to know where to put the modules. I think import is working in es modules, you don't need a bundler. (of course it won't work in non-modules like index.js but we can import from our other modules without problem).

Azgaar · 2022-06-20T19:44:55Z

Sorry, misclicked and closed the PR. Reopened

netlify · 2023-04-16T12:51:00Z

A new user left a comment. This user must be approved by a Netlify team owner before comments can be displayed.

Approve this user

alternative grid generators

3827a50

Azgaar assigned goteguru Jun 8, 2022

Azgaar self-requested a review June 8, 2022 07:19

tukkek mentioned this pull request Jun 9, 2022

Hex summary #829

Closed

goteguru added 4 commits June 18, 2022 13:08

Merge branch 'upstream' into dev-grid-generators

aaacd2d

ignore node modules (used for testing only)

b640f97

refactoring for row based code

db49f1c

update versions

4291777

Azgaar reviewed Jun 19, 2022

View reviewed changes

refactor and fix saved grid algo.

f4b3662

Azgaar closed this Jun 20, 2022

Azgaar reopened this Jun 20, 2022

Azgaar added the POC Proof of Concept label Jul 25, 2022

alternative grid generators #832

Are you sure you want to change the base?

alternative grid generators #832

Conversation

goteguru commented Jun 7, 2022

Description

netlify bot commented Jun 7, 2022 • edited

✅ Deploy Preview for afmg ready!

Azgaar commented Jun 8, 2022

tukkek commented Jun 9, 2022 • edited

goteguru commented Jun 9, 2022 • edited

Azgaar commented Jun 9, 2022

goteguru commented Jun 18, 2022

Azgaar commented Jun 19, 2022

goteguru commented Jun 19, 2022 via email

Azgaar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Azgaar Jun 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goteguru Jun 20, 2022 • edited

Choose a reason for hiding this comment

Azgaar Jun 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goteguru Jun 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goteguru commented Jun 20, 2022

Azgaar commented Jun 20, 2022 • edited

goteguru commented Jun 20, 2022

Azgaar commented Jun 20, 2022

netlify bot commented Apr 16, 2023

netlify bot commented Jun 7, 2022 •

edited

tukkek commented Jun 9, 2022 •

edited

goteguru commented Jun 9, 2022 •

edited

Azgaar Jun 19, 2022 •

edited

goteguru Jun 20, 2022 •

edited

Azgaar Jun 20, 2022 •

edited

goteguru Jun 20, 2022 •

edited

Azgaar commented Jun 20, 2022 •

edited