-
Notifications
You must be signed in to change notification settings - Fork 73
RFC: Backwards-compatibility policy and allowed changes #827
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,64 @@ | ||||||
| # Community Highlander Compatibility Policy | ||||||
|
|
||||||
| ## Behavioral changes | ||||||
|
|
||||||
| All Highlander changes can potentially cause a change in observable Highlander behavior. | ||||||
| Ideally, these changes are opt-in, either through config flags or events/hooks that | ||||||
| retain default behavior if not implemented by mods. However, bugfixes are exempt from | ||||||
| this policy, which can cause incompatibility: | ||||||
|
|
||||||
| * The most prominent example being the [`ArmorEquipRollDLCPartChance`](https://x2communitycore.github.io/X2WOTCCommunityHighlander/misc/ArmorEquipRollDLCPartChance/) change, | ||||||
| which can cause soldiers without torsos. We "fixed" it by offering an opt-out. | ||||||
| * The [`ScreenStackSubClasses`](https://github.com/X2CommunityCore/X2WOTCCommunityHighlander/pull/796/commits/61477511aad97b9da8aa989b4be8fddb85b50d22) change, | ||||||
| which can cause a change in behavior of the UI system. It was deemed worth it because it fixes a far greater amount of bugs. | ||||||
|
|
||||||
| Such changes must explicitly be documented with the `compatibility` tag. | ||||||
|
|
||||||
| ## Naming conflicts | ||||||
|
|
||||||
| Event or function names can potentially conflict with names chosen by mods. It is recommended that HL events or names | ||||||
| use a prefix if a conflict is likely (CH_) and otherwise restrict name choice to variations of existing names (for example a function | ||||||
| `DoXWhenY` that gets an event trigger may trigger the event `OverrideDoXWhenY`). | ||||||
|
|
||||||
| ## Compile-time-only changes | ||||||
|
|
||||||
| Sometimes we change functions or properties to remove the `private` or `protected` visibility modifier. | ||||||
| This isn't strictly necessary to do in the Highlander, as this can simply be done with local `Src(Orig)` sources, | ||||||
| and the UnrealScript runtime seemingly doesn't enforce visibility modifiers at all. | ||||||
|
|
||||||
| (Note: `private`/`final` functions cause static dispatch to be inserted by the compiler and cannot be overridden, | ||||||
| while-non-private/final functions can be overridden and their calls use virtual dispatch. This means that changing | ||||||
| a function to `public` in local sources and compiling against that causes newly compiled scripts to use virtual dispatch | ||||||
| and call the overridden function, but existing call sites (the original `XComGame` class) use static dispatch.) | ||||||
|
|
||||||
| ## Runtime-observable changes | ||||||
|
|
||||||
| ### Type changes | ||||||
|
|
||||||
| Changing the type signature of a function or a property (including removal) is a breaking change and causes a crash. | ||||||
| One exception is the addition of optional arguments; existing function calls (though not overrides in subclasses) are unaffected. | ||||||
|
|
||||||
| **Unresolved Question:** Does this extend to private functions or properties? Does it matter whether they are base-game provided or CHL-added? | ||||||
|
|
||||||
| For example, for [#434](https://github.com/X2CommunityCore/X2WOTCCommunityHighlander/pull/436/files#diff-3fd330f5f74f0a5f020891eb078df6b7R895), | ||||||
| an **existing** private function signature was changed to accomodate the event trigger added to the function: | ||||||
|
|
||||||
| ```diff | ||||||
| -private function int CalculateRiskChanceToOccurModifiers(int ChanceToOccur, bool bChosenIncreaseRisks, bool bDarkEventRisk) | ||||||
| +private function int CalculateRiskChanceToOccurModifiers(CovertActionRisk ActionRisk, bool bChosenIncreaseRisks, bool bDarkEventRisk) | ||||||
| ``` | ||||||
|
|
||||||
| For [#825](https://github.com/X2CommunityCore/X2WOTCCommunityHighlander/pull/826#discussion_r399667325), it is debated whether | ||||||
| the helper function **added by the same PR** should be `private` or not: | ||||||
|
|
||||||
| ```diff | ||||||
| -simulated function bool TriggerOnOverrideHitEffects( | ||||||
| +simulated private function bool TriggerOnOverrideHitEffects( | ||||||
| ``` | ||||||
|
|
||||||
| There are two competing arguments: | ||||||
|
|
||||||
| 1) People can *always* deprivatize functions locally and call them, and that's the suggested and preferred approach to many visibility | ||||||
| issues. Having any visibility modifiers gives us a false impression of the changes we can safely make. | ||||||
| 2) We have in the past [explicitly deprivatized](https://github.com/X2CommunityCore/X2WOTCCommunityHighlander/issues?q=is%3Aissue+label%3Ade-private%2Fconst) items in the HL. A very specific and not well-known hack in the Unreal Virtual machine is not covered by our backwards compatibility guarantee and only | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Uhhh why would anyone would ever want to do that to a vanilla function? I thought doing something like that would cause a crash on startup or something like that.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the discussion here: #826 Lines 912 to 913 in 45b7370
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would crash when someone deprivatizes the function and calls or override it in a mod. This is my main issue with doing that.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see. What would happen if it is called through a non-private wrapper?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can avoid changing function signatures e.g by calling new intermediate functions. this would be save to do. Theoretical example (don't know if its actually viable here):
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Not viable here because this is a pure math function. Best to simply keep the old function around but not use it) |
||||||
| precludes changes with evidence to break things. | ||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprivatize variables and functions is a tool that let modders circumvent limitiations when dealing with sub classes of e.g actions, targeting methods and mod class overrides. Since the usage of protected and private seems completely arbitrary and inconsistent in the code base this is often necessary. Also boundaries of native functions can make this necessary.
This method is highlander agnostic so it also can and is used by modders who don't use the highlander and possibly don't know about any rules and regulations about private scopes.
It is near to impossible to monitor the code base of all mods on the workshop to detect possible incompatibilities if we change private variables or functions signatures assuming its safe to do cause we as a small team of highlander devs defined private scope as off limits for all modders out there (which is also presumptuous imo).
I also see very little benefit in changing method signatures. You can avoid this by calling new intermediate methods to achieve the same thing. You have a little bit more boilerplate code but are sure you won’t break any mods.
So i strongly recommend treating private functions and variables the same as if they were public. As a matter of fact i would even deprivatize all of them to make this clear and to solve the virtual/static dispatch issue of deprivatizing private functions locally. This also has the benefit that modders who compile against the HL don't need to create pull requests for deprivatizing stuff all the time.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can totally agree with not changing type signatures of base-game private functions/properties. I do however have an issue with
If you very much know what you're doing and accept the risk of violating private invariants (as Iridar said "the value was originally meant to be private, and must be used outside of own class with the necessary level of care."), you can deprivatize things locally. If you want we can even host in a separate repo a script with a disclaimer that looks at the SrcOrig folder, rudimentarily parses the UC files and strips all visibility modifiers, but adding ~18MB of source files to the repo to only deprivatize everything is not something I can get behind.