search

tibiamaps / tibia-maps-script

:wrench: A command-line utility to convert between binary Tibia maps and human-readable forms of the map data.
https://tibiamaps.io/
MIT License
17 stars 8 forks source link

allow union/additive mode for markers #26

Closed rmobis closed 1 year ago

rmobis commented 1 year ago

Working on a few PRs on tibiamaps/tibia-maps-data I thought it would be really helpful to have a way to import markers additively, so I started working on this proposal.

Idea

The idea is to have a way to add new markers to an already existing minimapmarkers.bin or markers.json file, without overwriting old ones, by importing them from another file. We would add a new flag (--union, --keep-markers, etc.) that would signal the script that if the destination file already exists, it should not be overwritten but rather added to.

This can come in handy when importing new POIs made on old data, if I want to import all TibiaMaps markers but also keep my own or if a friend has some markers for some bestiary bombs and I want those imported along with mine.

Example

Below is a representation of files containing given markers, before and after running the following command with the new flag: tibia-maps --union --from-minimap=./minimap --output-dir=./extra/points-of-interest:

./minimap/minimapmarkers.bin

marker-a
marker-b

./extra/points-of-interest/markers.json (before running the script)

marker-c
marker-d

./extra/points-of-interest/markers.json (after running the script)

marker-a
marker-b
marker-c
marker-d

On Marker Duplication

We obviously want to avoid having the same marker multiple times, as that can even become a size issue with time. A simple equality check of all the marker properties should be enough to identify duplicities and is what I would recommend with, but we could look into other strategies later, such as checking only for the coordinates equality and allowing a single marker per sqm, if we find we need to. Can we even have more than 1 marker per SQM, technically?

There's probably a bit more to take into consideration, but I would love to hear what you think about the idea. I'd be willing to work on a PR, if you find it could be useful too.

mathiasbynens commented 1 year ago

Can we even have more than 1 marker per SQM, technically?

In the binary file format, technically yes, there could be multiple markers for the same sqm. But we definitely shouldn't do this in TibiaMaps.io data. I think we should actively dedupe markers.

Your proposal to add --union sounds great! Let's do it :)

rmobis commented 1 year ago

Awesome, I'll get started now. I've stumbled upon another matter I would need you attention, though:

While initially playing with the code, I wanted to run the tests to make sure everything was good before I get started, but I noticed the test is a bash script. Being on a Windows machine, that doesn't make running it very easy. I was wondering if, in favor of interoperability, you wouldn't consider having it converted to a JS script which could be ran anywhere Node runs. I can work on that in a different PR, just let me know.

mathiasbynens commented 1 year ago

Awesome, I'll get started now. I've stumbled upon another matter I would need you attention, though:

While initially playing with the code, I wanted to run the tests to make sure everything was good before I get started, but I noticed the test is a bash script. Being on a Windows machine, that doesn't make running it very easy. I was wondering if, in favor of interoperability, you wouldn't consider having it converted to a JS script which could be ran anywhere Node runs. I can work on that in a different PR, just let me know.

I'd be open to reviewing a PR for that, sure!