feat: more customization APIs

[moe-ad]

See #465 for context.

Summary of changes.

• I moved PyVistaBackendInterface._extract_kwargs to an helper module. The rationale is that we might need it in a lot more places in the future (if lessons from pydpf-core are anything to go by, there is a similar method that has found utility all over the place in the project's plotting classes).
• Added add_point_labels: a new API you can pass points and labels lists to annotate 3D point locations:

PyVista	Plotly

• Added clear: a new API to remove all actors from scenes, workaround to achieve consistent behavior added to the PyVista backend.
• Added examples and tests
• Fixed how add_text was used in pyvista examples (using pixel coordinates is tricky):

Before	After

[AlejandroFernandezLuces]

Thanks for moving the changes here!

On the add_point_labels, after looking into it, the main difference between adding a label and adding text, is the fact that the label will always be facing the user, while adding text will rotate with the scene. This is a reason good enough to have a public API for this. Although, the reason why PyVista calls it point_label is because in VTK you can associate labels to their different types of objects: points, edges, vertex, etc. So, I would rename it to simply add_label, to be as generic as possible. But agreed that this is a legitimate use case for the public API.

For the clear method, I still think it kind of leads to confusion for users. I think it leads to think that the flow would be something like this:

plotter.plot(mesh)
plotter.show()
plotter.clear()
plotter.plot(mesh)

which breaks for PyVista, our major backend. We could make a workaround in the PyVista backend by recreating the plotter instance after a show(), so externally you don't need to create another instance, but I feel this is incredibly hacky.

I understand your interactive argument, but it compromises scripting and coding in my opinion. What do yout think? Do you see any way of clarifying this for the users elegantly?

[moe-ad]

Thanks for moving the changes here!

On the add_point_labels, after looking into it, the main difference between adding a label and adding text, is the fact that the label will always be facing the user, while adding text will rotate with the scene. This is a reason good enough to have a public API for this. Although, the reason why PyVista calls it point_label is because in VTK you can associate labels to their different types of objects: points, edges, vertex, etc. So, I would rename it to simply add_label, to be as generic as possible. But agreed that this is a legitimate use case for the public API.

For the clear method, I still think it kind of leads to confusion for users. I think it leads to think that the flow would be something like this:

plotter.plot(mesh)
plotter.show()
plotter.clear()
plotter.plot(mesh)

which breaks for PyVista, our major backend. We could make a workaround in the PyVista backend by recreating the plotter instance after a show(), so externally you don't need to create another instance, but I feel this is incredibly hacky.

I understand your interactive argument, but it compromises scripting and coding in my opinion. What do yout think? Do you see any way of clarifying this for the users elegantly?

Hello @AlejandroFernandezLuces (You owe me one La Casera 🍹 when next we meet, I faced a rebasing hell this morning due to not syncing on time with all the subsequent changes you made to your PR):

• I can see you eventually dropped using pv.Text3D in your PR, seems like the right call.
• I can rename the API to add_labels no issues at all.
• For clear, it's going to incredibly hacky truly, especially tracking the config the user used for setting up the plotter and recreating the plotter with that exact same config. Besides documenting how it is expected to be used, I see no other way forward for elegant clarification for the users. If you think adding it doesn't bring much value, we can remove it. Let me know what you think.

[AlejandroFernandezLuces]

Sorry about that 😄

Let's keep the clear method and add specific documentation in the PyVista docs clarifying this issue. After discussing with @RobPasMue , we came to the conclusion that most of the backends we may end up implementing will benefit from it anyway.

I know that the PR is still in draft, but if you could add use examples for the new APIs, it would be great. Thanks for the hard work 🙂

[RobPasMue]

I faced a rebasing hell this morning due to not syncing on time with all the subsequent changes you made to your PR

Isn't that the PR's owner fault for not rebasing frequently.. ? 😄

PS: just kidding 😄 hope it wasn't too bad

[moe-ad]

I faced a rebasing hell this morning due to not syncing on time with all the subsequent changes you made to your PR

Isn't that the PR's owner fault for not rebasing frequently.. ? 😄
PS: just kidding 😄 hope it wasn't too bad

Picking sides... ☹️
You know what, you also owe me one La Casera, non-negotiable.

[moe-ad]

Sorry about that 😄

Let's keep the clear method and add specific documentation in the PyVista docs clarifying this issue. After discussing with @RobPasMue , we came to the conclusion that most of the backends we may end up implementing will benefit from it anyway.

I know that the PR is still in draft, but if you could add use examples for the new APIs, it would be great. Thanks for the hard work 🙂

Added!

[RobPasMue]

Letting some comments - thanks for the contrib @moe-ad ! Requesting changes though

[AlejandroFernandezLuces]

LGTM, couple of comments.

[moe-ad]

@AlejandroFernandezLuces I added e27ef09.

[AlejandroFernandezLuces]

Other that the renaming of helpers, LGTM. Thanks a lot for the contribution @moe-ad ! 🚀

[RobPasMue]

Small comments - thanks @moe-ad !