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

An option to enable a headless ActiveDoc #703

Open
wants to merge 3 commits into
base: 8.x
Choose a base branch
from

Conversation

spigach-tt
Copy link

Context

Many Grasshopper plugins rely on RhinoDoc.ActiveDoc for things like tolerances and units. By default, a Compute instance does not instantiate an ActiveDoc, which results in a NullReferenceException when plugins attempt to access its properties. There are workarounds that rely on the Grasshopper.Utility class, but those have to be implemented by the plugin's developer, which means that a number of third-party plugins cannot run on Compute.

Purpose

The purpose of this pull request is to give users the option to create a headless RhinoDoc.ActiveDoc when they launch an instance of Compute. This will allow Grasshopper plugins to access RhinoDoc.ActiveDoc properties without throwing an exception.

Description

The users will now be able to set a local environment variable called RHINO_COMPUTE_CREATE_HEADLESS_DOC. The value of this variable can be set to either true or false with false being the default. This variable is checked by the Load() method of the compute.geometry.Config static class and its value is assigned to the CreateHeadlessDoc property.

Each time the GrasshopperSolveHelper method of the ResthopperEndpointsModule is invoked, it checks whether Config.CreateHeadlessDoc is set to true. If it is, a brand new headless document is instantiated with tolerances and units determined by the input object. To clarify, a brand new ActiveDoc is created at a worker level for each Compute request.

Testing

This branch has been tested with both Hops and direct REST API calls from Python. The target Grasshopper script contained a C# component with calls to the ActiveDoc properties. Testing from Python was designed to flood several Compute workers with concurrent requests, each containing a unique combination of tolerances and units. Consistency between input ActiveDoc settings and the settings reported by the Grasshopper script was checked for each invocation to rule out possible conflicts between parallel ActiveDoc instances. No discrepancies or inconsistencies have been identified while testing under significant load.

image

@andyopayne
Copy link
Contributor

Hey Sergey. Thanks for putting together this PR. See my comments below:

  1. Your code never disposes the headless documents. Creating a doc and not disposing it will cause memory issues. Thus, I think you need to add the following lines just prior to the returnJson being sent back as the response (around line 117 in ResthopperEndpoints.cs).
if (RhinoDoc.ActiveDoc is object)
    RhinoDoc.ActiveDoc.Dispose();
  1. I think it would be a good idea to inform the user (in the console) that headless documents will be created/used. I think you may want to add something like this to Program.cs around line 48.
if(Config.CreateHeadlessDoc)
    Log.Information("Compute to use headless Rhino documents");
  1. I pushed a few commits to the 8.x branch on Monday. Please merge those latest changes into your branch and make the PR include those changes.

Let me know if you have any follow up questions or concerns. Cheers.

@spigach-tt
Copy link
Author

Hi @andyopayne! Thank you very much for your comments. I have implemented the changes that you mentioned: please, let me know if everything looks good to you.

With regards to incorporating your latest changes into my branch, both GitBash and GitHub Desktop tell me that my branch is up-to-date with upstream. I also do not see any new commits to 8.x ahead of what's already in my feature branch:

commits

If I am wrong about this, please let me know.
Thanks!

Best,
Sergey

@sbaer
Copy link
Member

sbaer commented Jan 9, 2025

Do you think it would be better to make this option available as part of the request json?

@spigach-tt
Copy link
Author

@sbaer Good question. Just thinking out loud here:

I guess the person hosting the compute server (let's call them the admin), is the one deciding which plugins to make available for the users calling into their instance (let's call them the clients). If the admin decides to host plugins that query the ActiveDoc, they have the option to enable this feature. The clients do not have any say in what plugins are available on a compute instance, so, arguably, there is no need for them to enable or disable the ActiveDoc.

There is only one scenario I can think of in which a client might want to unilaterally enable the ActiveDoc - if they have custom C# / Python scripts inside their Grasshopper definitions that need Doc access via RhinoCommon. Which is a legitimate use case, albeit a relatively less common one.

So let's consider these three options:

Admin-enabled only

When an admin deploys plugins that require ActiveDoc, they configure Compute to provide Doc access. No change is required on the client side: things work out of the box. However, in instances where an admin did not enable Doc access, clients cannot use custom scripts that call into the Doc.

Client-enabled only

An admin deploys the plugins that require ActiveDoc. They have to communicate to the clients that their POST requests must be modified if they want to use those plugins. Things will break by default unless the client makes the change. The upside is that clients can run scripts that call into the Doc regardless of the server-side configuration.

Combined approach (Admin-or-client-enabled)

Both approaches are possible. Admins can configure Doc access to make plugins work out of the box. Clients can still run scripts that call into the Doc by modifying their POST request regardless of server-side configuration.

I am leaning towards the last option, but it's really up to you guys. Let me know what you want to do.

@sbaer
Copy link
Member

sbaer commented Jan 10, 2025

Adding to the request can be done later. Let's just go with the original option to create headless docs.

On another note:
I believe a json request does have the ability to say what tolerances should be used. Is the document being created with these tolerances in mind?

@spigach-tt
Copy link
Author

Yup! I'm setting angle tolerance, absolute tolerance, and units based on the input object.

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 this pull request may close these issues.

3 participants