Banned Procedures for making changes (Baking Unitdefs)

[drivver44]

Work done

Unitdef baking creates significant burdens on everyone else who has open/draft PRs.

Unitdef baking has created issues where PRs have had to be fully redone due to someone using Unitdef baking to make changes.
See #3818 where this matter made significant headache causing Kroyla to kill and open a new PR.

Unitdef baking also kills all comments in Unitdefs.
I do believe it is possible to do unitdef baking while retaining comments but im not sure.
all cases of Unitdef baking that I have seen have resulted in comments being deleted.

[drivver44]

@sprunk @WatchTheFort for your awareness i cannot assign reviewers
WTF wants Sprunk to be the assigned reviewer

[WatchTheFort]

lmao I thought you were going to delete the def baking code, and wanted Sprung to review that

[sprunk]

At a basic level I agree but I don't have that much of a stake here, so just for completeness I'll list both the upsides and the downsides so that the actual decision makers (perhaps even admins rather than WtF? They all touch defs relatively a lot) can make a more informed decision. Note that by "baking" I mean using the baking script included with BAR which is a much more limited definition than the general definition of moving stuff from postprocessing to raw files (which is fine and often desirable if it's targeted and doesn't involve plowing through and upheaving the whole file and unrelated values).

The good things about baking:

• the values are subject to essentially no post-processing so there are no surprises regarding how a value ends up afterwards, such that the file is sufficient to be the entire source of truth. For example, if a unit says its wreck is 123 metal then you can be sure it really is 123 metal ingame regardless of anything.
• the file produced is fairly flat and has little computation involved, such that people e.g. pulling the def files off github can easily parse them and produce, idk, a unit database or something.
• the file contains even "derived" values, such that you don't need to do it manually from the actual "source of truth" value. For example if a unit says its wreck is 123 metal then you can just use that as opposed to having to know that wrecks are 456% of unit cost and calculating it yourself from cost.
• helps avoid post-processing from balooning up, which could increase load times.
• enforces a structure, which helps people navigate defs (e.g. currently it's alphabetical).
• some of the formatting-related downsides could be fixed.

And the reasons to remove:

• baking, as opposed to processing, loses the link between the values and the logic that produces them, or other values they depend on. If you bake "wrecks are 60% of original unit value" then sooner or later people will adjust one but not the other and then the rule gets broken. If this gets caught early on you can restore it, but subtle rules can become forgotten over time. For game-wide stuff like wrecks it's probably not a worry, but you can have "unique rules" e.g. a unit has two weapons and one is 80% of the range of the other for some design reason.
• baking, as currently implemented, loses comments. This also loses the important "why" aspect.
• as currently implemented, doesn't let you do computation. This includes even "nice" computation like using math.deg(45) instead of 0.78539816339, or highTrajectory = Trajectory.UserToggle as opposed to a magic 2.
• having health = Spring.GetModOption("TOUGH_peewees") and 785 or 230 makes it both very clear that the value is subject to change and on where to find the value if you want to touch the modoption.
• as currently implemented, implements some crappy formatting e.g. the { [1] = foo, [2] = bar } array style.
• as currently implemented, affects the whole file, putting the burden on the contributor to only commit the interesting parts.
• modders can still grab a nice dump of values via widgets which has the benefits of always being the "real" values regardless of whether there is processing or not, including processing done via engine which games have no control of.
• most of the things you'd want to do with baking can be done with core Unix utilities like awk or even sed. This is partially why I say "as currently implemented" above, in that I wouldn't mind baking if implemented via precise use of awk as opposed to the crappy existing script.

[drivver44]

• Watch the fort

lmao I thought you were going to delete the def baking code, and wanted Sprung to review that

TBH, I thought i was clear about the PR guidelines being the target of the PR as the process to ban something like unitdef baking has to be done at this level. I would rather stop behavoir from continueing rather than deleting already merged code and asking people to redo work, as doing that would mean that people would be doubly affected by merge conflicts.
All good though.

• Sprunk

• baking, as currently implemented, loses comments. This also loses the important "why" aspect.

We also lose the where aspect for a number of tags when comments stating where they are defined are lost.
A number of tags IE "usepiececollisionvolumes" , "tracktype" ,"selfdestructas", "weapontype" or ,"movementclass" are all tags that have a definition that is outside of the unitdef.
You would have to search the repo to find exactly where things are being defined in the repo whenever comments are deleted refferencing where a tag def will be.

[saurtron]

I do believe it is possible to do unitdef baking while retaining comments but im not sure.

For sure it can be done, baking is not bad per-se, but the baking code should be careful to not touch anything it doesn't need to change. ie, don't change capitalization, don't reorganize things, generally don't affect any lines not really needed to be changed.

Anyways I understand for some use cases it can be almost impossible without some automatization/baking, whoever needs that can ping me and we can find a better way.

[SethDGamre]

I do believe it is possible to do unitdef baking while retaining comments but im not sure.

For sure it can be done, baking is not bad per-se, but the baking code should be careful to not touch anything it doesn't need to change. ie, don't change capitalization, don't reorganize things, generally don't affect any lines not really needed to be changed.

Anyways I understand for some use cases it can be almost impossible without some automatization/baking, whoever needs that can ping me and we can find a better way.

By definition, baked defs are reformatted because they didn't adhere to a consistent format.

Exception: comments

[sprunk]

Seth has a good point. The wording of the rule should say something like "mass formatters" instead of "baking".

[WatchTheFort]

If we're going to say, "Don't use this single-purpose tool for the single purpose it was created for," then we should just delete the tool.

[drivver44]

• Sprunk

Seth has a good point. The wording of the rule should say something like "mass formatters" instead of "baking".

I'll update that then

btw everything you list here under reasons for removal #4172 (comment)

I view it as reasons to not use for now until these listed reasons have answers to resolve why the baking is bad.

• watch the fort

If we're going to say, "Don't use this single-purpose tool for the single purpose it was created for," then we should just delete the tool.

Ultimately the issue is that the tool is desctructive to unitdefs, the issues that are being raised are, to my knowledge, resolvable to a degree.

[saurtron]

By definition, baked defs are reformatted because they didn't adhere to a consistent format.

Don't think that's necessarily the definition of baked defs... You can bake without reformatting (just not with that tool).

[saurtron]

If we're going to say, "Don't use this single-purpose tool for the single purpose it was created for," then we should just delete the tool.

I wouldn't delete it yet, because it can be used as a step for a tool that merges changes without modifying uneeded lines, or it could have other uses, like to see what gets modified through some code.

Maybe simply add warning to the tool saying don't blindly copy the results into units/... etc?

[WatchTheFort]

Is this not already covered under

Each PR should address a single feature or bug.

[drivver44]

Is this not already covered under

Each PR should address a single feature or bug.

this is to cover a means of creating change
something like reformatters should be specifically meantioned as people may confuse what is and isnt allowed exactly by that line your refferencing.

[saurtron]

because it can be used as a step for a tool that merges changes without modifying uneeded lines,

here is the tool (well for now its more a script), https://github.com/saurtron/bar-unitbaker

still need a bit of work to make it more usable and add a couple missing features, but that's the idea I had when I said that, merges changes from the bar unitbaker back into the units/ folder in a clean way.

[Mitvit]

This is silly, you often can't even tell if a PR was made by baking or by other means. Just judge a PR on its merits.