sarbian / ModuleManager

176 stars 96 forks source link

Feature Request: Allow specifying file names in NEEDS block #144

Closed smartdummies closed 3 years ago

smartdummies commented 5 years ago

I would like to be able to specify a file name in the NEEDS

For example @PART[partName]:NEEDS[FILE[GameData\MODPath\Parts\partx.mu]]

This would allow me to make changes if a specific file was present instead of just the mod to allow for the possibility that people do prune items from the mods to improve performance

blowfishpro commented 5 years ago

It already allows directories, I guess some mods aren't structured in a way that allows that though (in your example for instance, the mod in question seems to be dumping everything in the Parts/ directory)

smartdummies commented 5 years ago

My main reasoning for this is I am trying to utilize the part variant feature introduced in 1.4 to combine the many supported engine in RO to a single part. I am currently coding so that it requires the mod to be present to create the variant with the model from that mod. My ideal situation would be to check if that file exists to avoid situations where people trim the mod to only include some parts and then there is a variant that does not have a model. Using a check for the directory would be an improvement for sure, the file level check would be the most granular to ensure the model exists

linuxgurugamer commented 5 years ago

It only allows a directory name directly under the GameData, it doesn't do multiple directories ie: @PART[BSLmk12SciencePod]:NEEDS[Squad/zDeprecated/Parts/Command/Mk1-2Pod] still fails on KSP1.7, which is when that directory was introduced

Kerbas-ad-astra commented 5 years ago

Actually, NEEDS can check paths now, since commit 4a7e3c8 (MM versions > 3.0.7). However, it can't check against PluginData folders, and I recall reading (maybe from one of your posts in the SXT thread?) that zDeprecated is hidden from the game database as well.

linuxgurugamer commented 5 years ago

So it really isn't checking paths, it's looking at the database to see if it's there. Which isn't great, given that I need to check that. Otherwise, at least get the documentation updated, to specify that it is really checking the database rather than actual files/directories on disk

linuxgurugamer commented 5 years ago

Well, for me, this is a useless patch. I just realized that it won't work given that the zDeprecated directory isn't loaded into the gamedatabase

blowfishpro commented 5 years ago

zDeprecated is treated the same as PluginData from KSP's perspective. No configs, models, or textures are loaded from it. What would be the purpose of checking NEEDS against it?

linuxgurugamer commented 5 years ago

Yes, I realized that, see my comment above. Very annoying, and a real pita to work around

7ranceaddic7 commented 3 years ago

Imma necro this to expand on the directory targeting. Is directory targeting only limited to NEEDS? Can this be extended to the pass-process? Would it not to do so in the race to be LAST/FINAL?

blowfishpro commented 3 years ago

It's limited to NEEDS. Of course you can put whatever you want in FOR and the patch will still run, doesn't matter if it's a directory name or not.

7ranceaddic7 commented 3 years ago

Yeah, I get that. I use :FOR[TranceaddicT] to tag my personal MMpatches when testing. So, what am I trying to get at ...

Here's a use-case for adding directory targeting to the pass-functions...

There are situations where mods are bundled under a single main folder either (1) by the mod author themselves (e.g., Nereid/FinalFrontier, Nereid/NanoGauges, Nereid/S.A.V.E.) or (2) by some unspoken convention (e.g., ContractPacks/... ).

For the following just pretend these are only part packs.

Say I created a mod that contains overlays of each planet for ribbons. I don't want to replace what Nereid has provided, just layer a stencil on top of the ribbon to represent a planet. First landing on Kerbin gets a stencil_Kerbin. First landing on Duna, stencil_Duna. As long as I'm the only one doing anything ...

I can target the ribbons in a patch with the conditional :NEEDS[Neried/FinalFrontier] and no harm, no foul.

But, what if Nereid decides, "Hey, that's a cool, but I think I can make better stencils."; so, here's a patch to my own mod that does this same thing. Naturally, he is gonna do something like Nereid/FF_Stencils. (Or, someone does it and shoves it in Nereid's folder (verboten, I know.))

Now, I kinda like what he/they did and don't want to destroy it, but I want my stencil layered atop his ...

I can't target in a patch with a pass :AFTER[Neried/FF_Stencils].

The same holds true for the ContractPacks example ...

I can target CleverSats with :NEEDS[ContractPacks/Cleversats]. (Again, we're pretending its a parts pack, right. Roll with it.)

But not, with :LAST[ContractPacks/CleverSats].

Why would anyone want this?

Currently the only targeting in a pass is :AFTER[targetMod]. But, that will hoover EVERYTHING in /targetMod while I only want to do my hocus-pocus put on /targetMod/targetSubMod.

MM only sees the GameData/targetMod level; so, multiple nested non-dll mods aren't seen. They MUST be the only folder under GameData.

Mods by directory (sub directories of GameData):
  ...

  CommunityResourcePack
  CommunityTechTree
  ContractPacks
  ContractRewardModifier
  CrowdSourcedScience

  ...
blowfishpro commented 3 years ago

:LAST runs regardless of whether the mod exists

Lisias commented 3 years ago

What I think is a mistake. If I specify a tag as the parameter of a ordering directive, it's because I want the thing patched around the thing I specified. If the thing does not exists, then it should not be patched.

Consistency is important.

Now I need to use :NEEDS on :LAST, but not on :BEFORE and :AFTER. I'm presuming I need to use :NEEDS on :FIRST too?

7ranceaddic7 commented 3 years ago

:LAST runs regardless of whether the mod exists

That makes no logical sense. If the mod doesn't exist why is MM performing any patching? Why even bother requiring a tag?

It also goes against published information ...

Patches for modname values in NEEDS, BEFORE, AFTER, LAST that don't exist are removed.

Sorry @blowfishpro, I just checked the code and unless I can't read code that is not correct.

blowfishpro commented 3 years ago

The documentation is inconsistent but the code shows that what I said is in fact the truth. The fact that that method always returns true means that it will never be removed.

I'll update the documentation.

To see why it was implemented this way, please see the original issue

The reason to accept a tag is to be able to specify order within :LAST

7ranceaddic7 commented 3 years ago

Well, I guess I can't read code. (Note-to-future-Study: C# => forces true.)

Still, you were leaning to BEFORE/AFTER. And it sat that way for 6mo. until the only "logic" presented was that Sigma wanted it as an analog to FOR. Is that how the real-world coders really work?

I find the decision-making to have been. I know y'all aren't going to do diddly about this, but it is irritating how inadequate the decision-making process is and how capriciously changes are implemented because a few decide they want something done.

Hell, this is the ONLY mod that doesn't package or maintain changelog. How is anyone not intimately involved supposed to know EXACTLY what has changed from one version to the next.

sarbian commented 3 years ago

Hell, this is the ONLY mod that doesn't package or maintain changelog.

You love hyperbole...

7ranceaddic7 commented 3 years ago

Where is it? There is nothing here. Nothing is in the zip. The only thing shipped in CKAN is the dll. image

blowfishpro commented 3 years ago

The idea is that cleanup should be done regardless. Based on many conversations with modders I'm convinced there also needs to be some equivalent of BEFORE/AFTER that does not imply NEEDS as well. So it would align with that if it ever exists.

As for the changelog, yes that could be improved. Most of us stick with what we inherit, which in MM's case is the changelog only being in the OP of the forum thread. But having it somewhere on Github would probably make it more accessible.

Lisias commented 3 years ago

The reason to accept a tag is to be able to specify order within :LAST

And about :FIRST ? I need to add :NEEDS on it too to avoid being applied when the target doesn't exists?

7ranceaddic7 commented 3 years ago

Most of us stick with what we inherit,

THAT is a prime example of very inadequate the decision-making that I'm talking about.

We put y'all up on a pedestal as coding-gods doing amazing work, which you do do. (Coding for profit AND fun has inspired me to completely change careers.) But, just because this is a hobby is not a valid reason to abandon professionalism. You wouldn't dream of telling your boss that.

Sorry, @blowfishpro, but where I come from, that is a cop-out, no professional should have that string of words in their dictionary.

blowfishpro commented 3 years ago

If you have substantive suggestions to make, I suggest you make them. Attacking the people involved is only loosing you social credit in this scenario.

Lisias commented 3 years ago

Attacking the people involved is only loosing you social credit in this scenario.

You are not being attacked, sir. You are being criticized. Get used to it. This is open source, and we criticize our way into better solutions for our users.

Lisias commented 3 years ago

Where is it? There is nothing here. Nothing is in the zip. The only thing shipped in CKAN is the dll. image

Not even the license is there, what breaches even the Forum rules.

7ranceaddic7 commented 3 years ago

If you have substantive suggestions to make, I suggest you make them. Attacking the people involved is only loosing you social credit in this scenario.

Don't play the social credit card game. THAT is a passive-aggressive attack on me as not having commentary worthy of consideration because I'm not a sufficiently capable coder.

Now, what I've done, it seems, is hit a sore spot. The maintainers of this mod have become soooo essential to the furtherance of KSP and extending its useful lifetime, it seems they believe they are beyond doing the soup-to-nuts essentials expected of other lesser beings.

Here's a substantive suggestion: come back down to Earth with the rest of us, please, Start holding yourselves accountable to the same standards (licensing, changelog, etc) others are held to.

Substantive Suggestion2: MM maintainers need to hold themselves to a far, far higher standard of care because MM is that from which all mod-life exists. Yes, yes, you can just say fcuk it and walk away from it at any time. (Petty greek gods play that game. How much social credit do you get for that? See, passive-aggression is easy.)

Substantive Suggestion3: When you make changes: don't break things. If you do be prepared to roll the change back and help fix what your intention will break.

Substantive Suggestion4: Fix the documentation. I've seen a lot of work go into the MM code. Docs, not so much. I've tried to help with explanations of log errors and someone was kind enough to add it to the sidebar.

Substantive Suggestion5: Develop a roadmap. What does the IDEAL MM look like? Does anyone know? You've said:

Based on many conversations with modders I'm convinced there also needs to be some equivalent of BEFORE/AFTER that does not imply NEEDS as well.

The novelty of stock KSP wears off ... QUICKLY. MM is the ONLY REASON KSP is still breathing between expansion releases. THAT is a heavy burden to bear that I do not envy. And, while it may feel like I'm being a PITA (and, yes, I probably am a PITA) it's not just to be a thorn. It's because I do actually care about this mod being the best it can be.

blowfishpro commented 3 years ago

Don't play the social credit card game. THAT is a passive-aggressive attack on me as not having commentary worthy of consideration because I'm not a sufficiently capable coder.

It's about your tone and how you present things, not your coding skills. And it doesn't really even matter how valid your suggestions are. If you present them rudely you're not going to get anywhere.

Since this issue has become a pointless dumpster fire I'm going to close it. Please open new suggestions in new issues. And be civil about it.

@smartdummies sorry your issue got hijacked to hell. If you have more to say about it specifically feel free to reopen this issue or create a new one.

sarbian commented 3 years ago

The only thing woth adding is that the licence is in the README.md which is in the zip and has always been.

7ranceaddic7 commented 3 years ago

Perfect.