refactor: Implement proposed fixes for MapData [skip ci]

[kristofbolyai]

This PR expects that #2373 is merged, and we agreed upon using zoom steps for our zooming value.

Quoting #2158 (comment):

feat: Make map features have a range of level, add foundation for custom (dynamic only) and json-parsed map visibility

• Label should treat the int value of level better
• We need to decide how to handle unknown/suitable-for-all on level in map attributes
• Visibility for zoom levels is missing; either fix here or as a follow-up

feat: add base versioning for forward-compatibility in JsonProvider (and also add a missing local-file provider loading method)

• JSON files should have a version field

I expect this PR to be controversial, at least partially. I am okay with that, in fact, I hope we can agree on something even better, in the end.

[magicus]

In a first quick glance, this looks good. But I need to take a better look when I'm more sharp and concentrated.

My main worry is if we are going to be able to retrofit the existing "zoom levels" into the new format. Have you experimented anything with that? Things like town names that does not show when zoomed in very much, but neither when zoomed out very much. Some discrepancy wrt to the current situation is okay, but overall "feel" of zoom visibility must be able to be expressed.

[kristofbolyai]

In a first quick glance, this looks good. But I need to take a better look when I'm more sharp and concentrated.

My main worry is if we are going to be able to retrofit the existing "zoom levels" into the new format. Have you experimented anything with that? Things like town names that does not show when zoomed in very much, but neither when zoomed out very much. Some discrepancy wrt to the current situation is okay, but overall "feel" of zoom visibility must be able to be expressed.

It's really easy to port it, you can count the steps with a scroll keybind, so you don't even need to code/debug anything.

[magicus]

Interesting approach. I'd still like to see both the old and the new on-screen at the same time to double-check that they fade in/out at about the same time.

[magicus]

Also, my weekend was a bit hectic. I thought I would have time to review this and the other map PR, but I had not. I'll try to do it as soon as I can, but it might take a few days.

[kristofbolyai]

Also, my weekend was a bit hectic. I thought I would have time to review this and the other map PR, but I had not. I'll try to do it as soon as I can, but it might take a few days.

Start with the other PR first. This is only changes that have no effect yet, since we have no data supplying to MapData yet (so there is also nothing to test..)

[magicus]

Let's get #2373 sorted out first, then I will come back and look at this. (Basically, it is just the zoom levels that still worry me.)

[kristofbolyai]

Let's get #2373 sorted out first, then I will come back and look at this. (Basically, it is just the zoom levels that still worry me.)

And @magicus, whenever you have time, here is the next step.

[kristofbolyai]

@magicus Hi. I've sent this ping to you on both Github and Discord. Please respond to the first one you see, and ignore the second one.

I've been approached by some people from our community, working on the same project. Coincidentally, both would benefit from the new mapdata system. Also considering that the 1.2.0 release is now done, I'd like to finish up map data too.

The reason why I am pinging is you to ask about the development process for this feature. I have a 3 options in mind, so please pick whichever suits you the best.
1, You still want to manage the map data refactor. I still write the code. Only downside is that the review cycle will get really long.
2, I take over management of the refactor. I won't wait for explicit review from you, but I'll label every map-data PR or tag you, whichever you prefer, so you can do any post-review suggestions. Before the "final" release, I'll try to get your overall opinion on the change, if it's possible.
3, You'll have time to work on the feature in the upcoming time-period. If so, I'll take up another big project.

Some details about the "project" I've mentioned above:
This part was asked to not be public, and is only shared in the discord message.

[kristofbolyai]

@magicus I think I did everything we've discussed.

[magicus]

I'd like to see this somewhat different. The change I would like to see is that the fading should be "centered" around min and max, instead of extending the visibility. That is, if you change the fade it should still feel like the icon is shown for roughly the same interval. With this code, increasing the fade will make the icon visibile for a much longer period, and that will likely make people set a shorter visible interval to get the desired effect.

So basically what I am saying is that the max/min specifies the interval where the icon is shown with at least 50% visibility.

[kristofbolyai]

Is that desirable behavior, though? Personally, I would expect min max values a point where they either start or stop fading. Now, it's a mix of both, which I agree can be a bit confusing, but overall, I thought this makes the most sense, without having to read "documentation" for the user.

[magicus]

I think that is desirable, yes. Here are my two arguments:

1. This decouples fading from the actual max/min point. Otherwise you will probably need to adjust max/min at the same time you adjusts fading, to get your desired effect.

2. If you don't select the midpoint of the fading range, then what do you select? When the point starts to fade or stops to fade? It is kind of impossible to know.

My idea is to think of this as without fading. Then there is a single instance at which the visibility kicks in (or out). But the fade argument makes this a little less abrupt. But if you slide the fading up or down, it should still center around the actual value.

[magicus]

So can we set -1 as the combat level at the "root"? So if someone adds a data point and leaves out the level, the default value will be "no info".

[kristofbolyai]

Every other method here specified their default as inherit. I think this method should do the same, and we'll specify the real default value somewhere else (which will be called when the root is "inherit".

[magicus]

Ok. I've apparently forgotten like everything about this. :) Where do we specify the root? Can we make sure this is set to -1?

[kristofbolyai]

I am a bit lost in that regard still. Here is what I've found. It seems that FullFeatureAttributes and FullCategoryAttributes expect inherit values to be null, or 0. I would assume these classes would provide/get the default values. Somewhere, sometime. I think i'll have to figure this out, when I am working on porting to map data.

(For reference, here is a code snippet from FullFeatureAttributes:

    protected <T> T getAttribute(Function<MapAttributes, T> getter) {
        // Check if the feature has overridden this attribute
        if (attributes != null) {
            T attribute = getter.apply(attributes);
            if (attribute != null && !(attribute instanceof Integer i && i == 0)) {
                return attribute;
            }
        }

        // Otherwise get it from the category
        MapAttributes categoryAttributes = Services.MapData.getFullCategoryAttributes(feature.getCategoryId());
        if (categoryAttributes == null) return null;

        return getter.apply(categoryAttributes);
    }

[magicus]

This is the one point that is still worrying me. I think we need to get to the bottom on how this is supposed to work, so we get this part right.

Maybe it is bad that we use 0 for inheritance on the other integer values as well. Perhaps we need a way to say if an integer value is present or not, basically having nullable Integers instead of int. That is harder to match to a json format, though.

[kristofbolyai]

So, do you have any idea on how to handle this? Nullable ints are not so bad to handle in GSON, afaik.

[magicus]

See if you can revert this.

[kristofbolyai]

Only by force pushing. Do you want me to?

[magicus]

Yes, I think it is okay to force-push to a PR branch to get rid of a thing like this. A bit annoying that this is the only way to fix this, but in this case I think it's fine. I assume you'll make an interactive rebase and remove all edits to this file?

[magicus]

This has been superseded by #2489.