endless-sky
endless-sky copied to clipboard
endless-sky
outfit display name
Feature: This PR implements the feature request detailed and discussed in issue #7052
Feature Details
adds the tag "display name" to outfits so that two different outfits may have the same name.
Performance Impact
No performance impact
Anything to do now? I want this thing to be finished asap. Because I have some other things going on.
Looks like the compilation error is because the ByName comparator expects a Name function, which we've removed from Outfit. Seems like a relevant bit of information for #7063. For now, I'd suggest copying ByName into a new comparator called ByDisplayName that calls a DisplayName on T, and using that in MapOutfitterPanel where ByName is currently being used.
I will leave SetName as it is because otherwise I would have to change all occurences of SetName in other classes and that is not part of this pr
I don't really feel that this change is a good idea at the moment.
Allowing outfits to say they have the same name seems innocuous, but it's ripe for user confusion. Any place where we currently have a list of outfits, we will inevitably show a list that has multiple of the same name. Wherever we currently use the outfit name to indicate to the player the outfit currently being referred to, we will now provide insufficient context to let the player identify exactly which outfit we mean. These sites will generally then need to include the image sprite, but there is no guarantee that the two outfits with the same name have different sprites, either!
I definitely disagree with allowing someone to tell the game "render absolutely nothing in the place where we identify this outfit to the player."
Well yes I just resolved the issue, if this is not wanted I will close this again.
Allowing outfits to say they have the same name seems innocuous, but it's ripe for user confusion. Any place where we currently have a list of outfits, we will inevitably show a list that has multiple of the same name.
Those "confused users" would not be the result of the engine supporting this feature, but would be the result of the content-creators creating confusing similarly named stuff.
Having a "florpwax missile launcher prototype A" and "florpwax missile launcher production version C" both with the same name should be fine, since the stats will likely be different. (But a content-creator should aim to have both fire the same type of missile.)
I agree that we should not support DisplayNames for Outfits that can be used as Ammo (and we should pro-actively remove such displaynames from Ammo, with a warning when we do that).
I'm fine with not supporting empty names, but I'm failing to see what the exact issue is with ammo having a display name. Is that not just a similar matter of whether or not a content creator confusingly names an outfit/set of outfits? (#341 feels related.)
Ditch the booleans for whether the names have been set and just return to checking string.empty(), as at least that change is agreed upon.
I'm fine with not supporting empty names, but I'm failing to see what the exact issue is with ammo having a display name. Is that not just a similar matter of whether or not a content creator confusingly names an outfit/set of outfits? (#341 feels related.)
Yes, they are very similar and it is the responsibility of the content-creator to use this feature wisely. But the support requests we will get for confusingly named regular outfits will be simple to handle ("Hey, I have 2 outfits with the same name, but different stats"), while the support requests for confusingly named ammo are likely more difficult ("Hey, I have a weapon and I have ammo for it, but the weapon won't fire.")
The solution here is quite simple; if we trust our content-creators (and that is not just you, but also the content-creators that are less deeply involved in the code) to use the feature wisely, then we should implement it for all outfits (including ammo). If we are worried that the feature will lead to many support-requests, then we should implement only part of the feature (or not implement it at all).
Btw, issue https://github.com/endless-sky/endless-sky/issues/7052 is not specifically asking for multiple outfits with the same name; a single outfit with conditional attributes might be a better solution for the problem described in 7052.
This doesn't mean that this is a bad change, it only means that this PR doesn't fix the problem described in 7052.
Btw, issue https://github.com/endless-sky/endless-sky/issues/7052 is not specifically asking for multiple outfits with the same name; a single outfit with conditional attributes might be a better solution for the problem described in 7052.
This doesn't mean that this is a bad change, it only means that this PR doesn't fix the problem described in 7052.
I would like for there to be an optional attribute for outfits (and possibly ships) called "name," allowing you to set the name of the outfit as it displays in game, instead of always using the name as defined by whatever comes after "outfit" in the outfit definition. That's what he said
Ditch the booleans for whether the names have been set and just return to checking
string.empty(), as at least that change is agreed upon.
done
Btw if this gets merged engines would no longer needed to be sorted alphabetical... More naming options
Three intended use cases here:
- Plugin creators (such as myself) who would like to create an alternate start where outfits behave very differently.
- Engine, shield, etc. naming, so we no longer have to make everything alphabetical.
- Situations (like with the Kestrel) where you get an outfit unlock as a reward for a mission but can then choose which version of it you want.
I would have mentioned this earlier, but when I saw #7063 I thought this had already gotten merged, and I only found out today that it hadn't been.
I'd love to see @tehhowch look over this again.
