I am running into issues where MapAttribute is too close to MapLocation, and other implementations, like MapArea need a custom set of different attributes. I think we should consider different attributes for each MapFeature type. I also know it will be useful when implementing MapPath.

I've started working on this, but hit a roadblock. I'll have to either resort to option three, or think a little more. We can't solve this issue with attribute generics or inheritance, because categories need to be able to override all three (or any number of) map features. It would mean we have to have separate "getLocationAttrbiutes", "getAreaAttributes", etc, methods for categories, in which case, we might as well not use generics, and make every feature return free-standing attributes. I am not comfortable with either of these solutions, so I'll think about this a bit more.

Oh. Hm. I think I see the issue. Do you think there is any set of common attributes, or do they need to be completely separate?

There is definitely a set of common attributes. Like the proposed marker options. Maybe even the label, it can be rendered for all types, although paths requite a bit of creativity.

Ok. I think we have two possibilities, either make like MapLocationAttributes inherit and extend MapAttributes, or let MapLocation have a second method getLocationAttributes that returns a free-standing MapLocationAttributes.

Both solutions have pros and cons. The former might seem cleaner, but we will need to have consistency checks to make sure e.g. an area does not try to implement location attributes. The second is probably simpler to implement, but it also result in a kind of arbitrary split between two kinds of attributes. Especially in json I assume this will look weird, were some attributes will be under "attributes" and other under "locationAttributes".

Can there be a third option, perhaps? That MapAttributes include all attributes that are relevant for some form of map feature, and we just ignore those that are not relevant? So if like we're drawing a line then maybe iconId is irrelevant, that just means we will ignore that attribute, and providers do not bother definiting it, but if they did, it would not be an error.

The third option makes it so it's not clear what applies and what doesn't to the relevant feature, and can only be known after explicitly reading comments. The second option is problematic in the JSON representation, I agree. The first option would require either weird generics, or many "alike" classes.

Perhaps we could do option two with some GSON magic? What do you think? I can try to sketch up a PR to demonstrate (and/or realize its downside).

If we go by route 3, we can add some checking when reading a json file and emit a warning if irrelevant attributes are used, perhaps?

Or wait, we can't do that. Let's say you define a category mystuff:lootruns, and then you set both location and area specific attributes to it. And then you add some lootrun locations, and some lootrun areas. We can't warn for the category since we do not know how it will be used, and we can't warn for the location since the area attributes are in fact correct for the area features.

Actually, I think this makes for a rather powerful argument that route 3 is indeed the correct one. It seems reasonable that we should be able to define categories that can have attributes that are applied to locations as well as paths and areas. So it makes sense that we should have attributes that are not always applicable. We will just need to make sure we document properly where and how each attribute applies.

What I would go for, is having separate attributes for each type, in code. Then, we use Gson for handling json categories so they have "merged" attributes. Features don't need special care, they each have representative attribute classes. We only have to care about name conflicts, which becomes an explicit error when writing the deserializer, so it can't stay a lingering problem.

If you agree, I can PR this sketch today, and we can finalize it in the upcoming days.

Unless we internally also want to have categories which provide attributes to multiple classes of features. Like lootruns, it might make sense to have that, right?

I'm also in general reluctant to let the json format and the internal representation diverge. I think there is much point in keeping the json a 1-to-1 representation of the internal state, and I have tried to make sure all designs makes both the json and the internal design "feel" good and natural.

Okay. That means, we have to make categories have separate attribute fields, like areaAttributes, locationAttributes, and have them be a separate type.

Do we? I thought it meant that we could add location-specific, area-specific and path-specific attributes to MapAttributes. At least that was what I was thinking.

Do we? I thought it meant that we could add location-specific, area-specific and path-specific attributes to MapAttributes. At least that was what I was thinking.

I've went ahead and used the different class approach. I can publish a PR soon, but my main reason is that this way we can have "general" attributes defined in a superclass, and also have specific implementations. I've also decided to take a different approach then we usually take, and not use generics, rather rely on some basic/safe class-casting.

This was fixed in #2582