Conversation
...to a "push" architecture
presumably, multiple properties will be set
|
@gotmachine: I've been working on this for a while. Do you think it better belongs here, or in KSPCF? It does touch/fix stock issues, but I feel like the approach is too invasive/not enough of a 1:1 replacement for KSPCF. |
|
Well TBH I think Shabby as a whole could be part of KSPCF. Both mods have broadly similar goals and are touching similar stuff in the same way. As for this specific PR, you're likely better informed than me to make that call. I'm not opposed to radical modifications to the stock implementations, as long as stock end user facing stuff is untouched and that the modding ecosystem fallout is assessed and either inexistant or manageable from our side. |
|
Yeah that's fair re. Shabby/KSPCF overlapping. In this particular instance my main hesitation is that any mod which directly writes to Mods applying their own MPBs to the part hierarchy will also break, but I am not as concerned about this, because it's rather difficult to do that correctly and this is intended to address that. Technicolor will be the first external consumer of this API. This seems to be a bit too much breakage for my liking, but if you think differently we can move it. But in either case, I would appreciate a code review from you if you have time, particularly around Harmony patching and garbage management. |
|
Shabby's scope has certainly expanded, and some of the newer stuff does seem like a better fit in KSPCF. Can those new parts be cleanly removed from the rest of Shabby (Shader.Find replacement) ? But ultimately inertia will probably win out here and we continue down the current path; that's fine too. |
Yes, |
|
Note: the material duplication patch should be moved to KSPCF before merging. |
Phantomical
left a comment
There was a problem hiding this comment.
Having read through this, the code LGTM.
If I understand everything correctly the way you use it is:
- You create a new
Propclass, then set its priority and relevant properties - You then register the
Propsinstance withMaterialPropertyManager - Everything else then works from there on out.
The one problem is that if anyone ever touches Part.mpb then stuff breaks? I was originally a bit more leery of requiring anything that ever uses a material property block to go through Shabby, but now I'm kinda hoping this can be used to fix KSPModdingLibs/KSPCommunityFixes#371.
|
Yes, that's about right. Though the consumer should have some kind of dtor to unregister its
Yeah, I intentionally make touching This is because I am largely convinced that short of doing what this PR does, there is no way to use The linked MCC bug is already fixed by this PR; having multiple MCCs is well-known to be broken among part modders. There are a few cleanups I need to do to this PR; will see if I can find time this weekend. |
A complete overhaul of how dynamic shader properties are handled for parts. Rewrites how vanilla uses (read: attempts to use, largely incorrectly)
MaterialPropertyBlocks, and also provides an API for other mods to apply their own MPB-based changed without overwriting the vanilla features.In brief, vanilla uses a single part-level MPB to implement the following:
Part;MaterialColorUpdater(which, astoundingly, accidentally instantiates all of a part's materials due to callingRenderer.materialinstead ofsharedMaterial);HighlightingSystem, independent of the part's mesh);ModuleColorChangers. How this works given that the MPB is never reset is unclear to me.This PR Harmony-patches all of the above usages to become consumers of the new API.
Custom properties are specified using instances of the
Propsclass, which offers an MPB-like API. In addition, there is aPriorityfield, which determines precedence when multiplePropsset the same property on the same renderer.There is a singleton class
MaterialPropertyManager.Instance, which can be used toRegisteraPropsclass to be applied to a particular renderer. This only needs to be done once per renderer, and a single instance ofPropscan be registered for any number of renderers. Any subsequent changes made to thatPropsinstance will be transparently applied to its registered renderers, inLateUpdate. Reusing a singlePropsfor multiple renderers is encouraged, since all renderers sharing the same set ofPropswill share the same underlying MPB.Multiple
Propsregistered against the same renderer will be combined, with conflicts resolved by order of highestPriority. Properties set by vanilla code, as listed above, have priorityint.MinValue + 1, except for those set by MCC, which have priority0.