Skip to content

Material property management#24

Open
al2me6 wants to merge 30 commits into
mainfrom
mpb
Open

Material property management#24
al2me6 wants to merge 30 commits into
mainfrom
mpb

Conversation

@al2me6

@al2me6 al2me6 commented Jun 8, 2025

Copy link
Copy Markdown
Collaborator

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:

  • Making unattached parts semitransparent in the editor, directly implemented in Part;
  • Coloring overheating parts orange in flight, implemented in MaterialColorUpdater (which, astoundingly, accidentally instantiates all of a part's materials due to calling Renderer.material instead of sharedMaterial);
  • Implementing the "rim highlight" effect when hovering/highlighting a part. This is a Fresnel-like effect that tints the part mesh at grazing angles, separate from the glow effect that surrounds the entire part (which is implemented by HighlightingSystem, independent of the part's mesh);
  • Implementing the glow effect on certain asteroids and comets;
  • And implementing arbitrary changes made by any 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 Props class, which offers an MPB-like API. In addition, there is a Priority field, which determines precedence when multiple Props set the same property on the same renderer.

There is a singleton class MaterialPropertyManager.Instance, which can be used to Register a Props class to be applied to a particular renderer. This only needs to be done once per renderer, and a single instance of Props can be registered for any number of renderers. Any subsequent changes made to that Props instance will be transparently applied to its registered renderers, in LateUpdate. Reusing a single Props for multiple renderers is encouraged, since all renderers sharing the same set of Props will share the same underlying MPB.

Multiple Props registered against the same renderer will be combined, with conflicts resolved by order of highest Priority. Properties set by vanilla code, as listed above, have priority int.MinValue + 1, except for those set by MCC, which have priority 0.

@al2me6 al2me6 self-assigned this Jun 8, 2025
Comment thread Source/.editorconfig
@al2me6

al2me6 commented Jun 15, 2025

Copy link
Copy Markdown
Collaborator Author

@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.

@gotmachine

Copy link
Copy Markdown
Contributor

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.

@al2me6
al2me6 marked this pull request as ready for review June 15, 2025 21:31
@al2me6

al2me6 commented Jun 15, 2025

Copy link
Copy Markdown
Collaborator Author

Yeah that's fair re. Shabby/KSPCF overlapping. In this particular instance my main hesitation is that any mod which directly writes to Part.mpb will break. I am aware of two mods that do this: LazyPainter, for highlighting parts of the mesh that can be recolored, and KAS, for highlighting pipes.

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.

@JonnyOThan

Copy link
Copy Markdown
Contributor

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.

@al2me6

al2me6 commented Jun 17, 2025

Copy link
Copy Markdown
Collaborator Author

Can those new parts be cleanly removed from the rest of Shabby (Shader.Find replacement) ?

Yes, MaterialReplacement is independent of shader loading. However I put it here instead of in KSPCF because it seems useless without external shaders to replace.

@al2me6

al2me6 commented Jun 20, 2025

Copy link
Copy Markdown
Collaborator Author

Note: the material duplication patch should be moved to KSPCF before merging.

@Phantomical Phantomical left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having read through this, the code LGTM.

If I understand everything correctly the way you use it is:

  • You create a new Prop class, then set its priority and relevant properties
  • You then register the Props instance with MaterialPropertyManager
  • 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.

@al2me6

al2me6 commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

Yes, that's about right. Though the consumer should have some kind of dtor to unregister its Props instances.

if anyone ever touches Part.mpb then stuff breaks

Yeah, I intentionally make touching Part.mpb a no-op.

https://github.com/KSPModdingLibs/Shabby/pull/24/changes#diff-3fe6ea7fec416364a64dc3ac83808555e5abfaaf8feab88002cb3e251a48ed24R114-R115

This is because I am largely convinced that short of doing what this PR does, there is no way to use Part.mpb correctly. Beyond the known FAR breakage I'm only aware of some kind of pipe connection highlighting thing in KAS that would be impacted, though I am not sure how many people still use that. I think CD works with this installed, but c/should be rewritten to depend on this.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants