StrataSource / Engine

Issue tracker for Strata Source
47 stars 2 forks source link

Remove "unused keyvalue" check for problems warnings #865

Open vrad-exe opened 1 year ago

vrad-exe commented 1 year ago

What would this enhancement be for?

Hammer

Describe your enhancement suggestion in more detail

The Check for Problems dialog in Hammer will show a warning if an entity has an "unused keyvalue" (AKA keyvalue which isn't in the FGD). This is very rarely ever an actual problem, and in some cases (when a mapper is deliberately turning off SmartEdit to add a keyvalue not in the FGD), "fixing" the issue would actually break the map. Since it's also fairly common for maps to have old keyvalues leftover from earlier FGD versions or the like, this will cause the check for problems dialog to become filled with useless entries which make it harder to find actual problems. These warnings should just be removed.

ozxybox commented 1 year ago

This has been fixed in the latest Momentum public update and P2:CE staging update. Thank you SCell!

vrad-exe commented 1 year ago

The new version of this is not really useful and if anything it's worse. The warnings are still there, but now it wants to "fix" the problem by copying the value of the non-FGD keyvalue to another similarly-named one which is in the FGD...which seems even more likely to end up breaking things than the old behavior of removing it entirely! I also don't see when this "spell checking" behavior would ever actually be useful unless you're editing VMFs directly in a text editor, in which case you probably won't be looking at Check for Problems anyway.

It also still doesn't solve the second problem I mentioned of Check for Problems being cluttered up with irrelevant warnings when opening maps made with old FGD versions. HammerAddons (and by extension our FGDs) remove a lot of irrelevant keyvalues like Xbox specific things, keyvalues disabled in code, etc. so opening pretty much any map made for base P2 is going to fill Check for Problems with these warnings despite it being completely harmless.

I still stand by my original suggestion that this should just be removed entirely.

SCell555 commented 1 year ago

String comparing is done with Levenshtein distance. I set the distance for matched strings to 3. I could shorten the distance to 1, so it doesn't match that many false positives I guess.

vrad-exe commented 1 year ago

I still fail to see the purpose of having this feature at all. Hammer already lists all keyvalues which are in the FGD regardless of if they're set or not, you never have to manually type in keyvalue names unless you're adding a keyvalue which isn't in the FGD, in which case this still won't help you because Hammer won't know the correct name to begin with.

If the verification were instead done to AddOutput, comp_kv_setter, or other non-standard places which do require you to manually type in keyvalue names, then perhaps it would be more useful, but currently it does not apply to those.