Skip to content
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

Exposing the qualified name version of BeforePass #92

Closed
poi-vrc opened this issue Dec 27, 2023 · 6 comments · Fixed by #109
Closed

Exposing the qualified name version of BeforePass #92

poi-vrc opened this issue Dec 27, 2023 · 6 comments · Fixed by #109

Comments

@poi-vrc
Copy link

poi-vrc commented Dec 27, 2023

Hi,

There is a public overload version of BeforePass in the DeclaringPass class which accepts a generic type.

However, the version of qualified name string version of this is made private. I want to request to make this API visible, as I need this to specify a pass dependency that doesn't use the default qualified name.

The generic type method's API docs is also incorrect. I think it is originally for the string version?

https://github.com/bdunderscore/ndmf/blob/main/Editor/API/Fluent/Sequence/Sequence.cs#L85

@bdunderscore
Copy link
Owner

Hmm. My thought here was that I don't want to allow arbitrary passes to be referenced (as they're probably a private API); you should generally either declare a dependency on the plugin, or ask the plugin author to expose a public API that gives you a Pass instance. Is that not sufficient for your use case?

@poi-vrc
Copy link
Author

poi-vrc commented Dec 27, 2023

The qualified name overload is available for the AfterPass one, but explicitly marked as private in BeforePass: https://github.com/bdunderscore/ndmf/blob/main/Editor/API/Fluent/Sequence/Constraints.cs#L98

For the specific use case, it's to connect DT/DK to MA/NDMF. Details would be better to be discussed elsewhere.

image

I would need to find a way to allow my passes (which are dynamically created as wrappers) to create dependencies on NDMF plugins.

@bdunderscore
Copy link
Owner

Hmm, that might be an oversight that that’s public. I guess the question I have is - what third party passes do you want to declare dependencies on? If those are MA passes then MA might need to expose them as a supported API.

@poi-vrc
Copy link
Author

poi-vrc commented Dec 27, 2023

Let me clarify the situation first. I am creating a layer on top of NDMF which makes my tool can use the same API and features without able to feel what is MA or NDMF.

The constraints and dependencies structure of DK don't use the NDMF concept of singleton passes and automatically initialize using the generic type parameter. They are dynamically created and I am probably not changing this concept or inheriting the existing stuff to NDMF's Pass class. But to use a dynamic wrapper class to wrap each DK Pass instead to achieve NDMF integration.

Constraints are separately built and they all end up as list of pass identifier strings. What I am trying to do is, to have an API at my side to add NDMF plugin constraints using types typeof(MenuInstallPluginPass) (which actually converts back to the full type name) or qualifier strings for flexibility.

Changing BeforeRuntimeHook to accept NDMF's IPass<T> and pass it to BeforePass/AfterPass in NDMF doesn't look good for me because it will introduce the concept of NDMF to DK.

This is an example code of DK BuildPass trying to add a NDMF dependency.

public class DKToMAGenerationPass : BuildPass
    {
        public override string FriendlyName => "DK to MA Conversion";
        public override BuildConstraint Constraint =>
            InvokeAtStage(BuildStage.Transpose)
                .BeforeRuntimeHook("nadena.dev.modular_avatar.core.editor.plugin.MenuInstallPluginPass")
                .BeforeRuntimeHook("nadena.dev.modular_avatar.core.editor.plugin.RenameParametersPluginPass")
                .BeforeRuntimeHook("nadena.dev.modular_avatar.core.editor.FixupExpressionsMenuPass")
                .Build();

If the API isn't available for public, I can only use reflection to access it, which is actually not a very good way.

@bdunderscore
Copy link
Owner

Hmm. I don't have a strong objection to being able to name passes using qualifiers within your own plugin. However, for cross-plugin use cases, I think it's important to discuss the dependency you're adding with the other plugin author, to ensure that you're relying on a stable API surface instead of something that might be removed/refactored later.

In your case there, I consider those specific names to not be a public API (I reserve the right to rename/restructure internal classes), but I'm happy to add more targeted stable hooks to MA to allow for you to reference those passes in a stable way. Can you please file some issues on the modular avatar side for the specific places you're looking to add hooks before/after? It'd be helpful to have a description of why you need the hooks as well, as I might want to define the API in a more general way.

I probably won't be able to address this until mid-January, note.

@bdunderscore
Copy link
Owner

I'll plan on exposing the qualified name APIs, but I'll explicitly exclude the MA names from any version stability policy. Please file feature requests on MA for the specific passes you want exposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants