Load options, implementing #346

davidlatwe commented 4 years ago

What's changed

Implemented a new menu action, OptionalAction, a subclass of QtWidgets.QWidgetAction
Changed Loader SubsetWidget menu item from QAction to OptionalAction
Implemented load option dialog
Implemented a few option input widgets that can be used by loader plugin

Optional loading action

After revisit issue #346, I've decided to implement a dialog that pop-up from context menu to display the option controls, so the layout of current Loader GUI won't have to change much.

The dialog implementation was referenced from Maya's menu option box, and the option box will be shown on menu when the loader plulgin has options attribute. And since supporting multiple or even different subsets could lead to unnecessary complexity, the option box only visible when there's only one subset being selected.

The loader options should be a list of pre-defined input widgets which implemented for certain type of value and the input value will be collected and parsed into api.load(options=options).

Example usage

import avalon.api
from avalon.vendor import qargparse

class OptionsDemo(avalon.api.Loader):

    label = "OptionBox Demo"
    icon = "bomb"
    color = "#FEBB08"
    representations = ["*"]
    families = ["*"]

    options = [
        qargparse.Boolean("iAmHuman", help="Are you a human ?"),
        qargparse.Integer("age", default=15, min=0, help="How old are you ?"),
        qargparse.Float("height", default=170, help="How tall are you ?"),
        qargparse.Double3("position", default=(0, 1, 0), help="You're location cord."),
        qargparse.Enum("sex", ["Male", "Female"]),
        qargparse.String("message",
                         placeholder="Say something",
                         help="Leave a message."),
    ]

    def load(self, context, name=None, namespace=None, options=None):
        for key, value in options.items():
            print(key, value)

optional_actions

In Production (Testing)

# Pseudo code

class MyLoader(api.Load):

    options = [
        qargparse.Integer("count", default=1, min=1, help="Batch load count."),
        qargparse.Double3("offset", help="Offset loaded subsets."),
    ]

    def load(self, context, name=None, namespace=None, options=None):
        # Before load
        options = options or dict()
        count = options.get("count", 1)
        if count > 1:
            options["count"] -= 1
            self.load(context, name, options=options.copy())

        # Start loading
        ...

        # Offset loaded subset
        if "offset" in options and cmds.objExists(root):
            offset = [i * (count - 1) for i in options["offset"]]
            cmds.setAttr(root + ".t", *offset)

optional_load_rig

Any feedbacks are welcome :relaxed:

mkolar commented 4 years ago

The idea and implementation look very good. Not in the way when not needed and quick to use when it is. I like it.

mottosso commented 4 years ago

Yes! I would write it in much the same way; API is declarative, handling of options is non-obtrusive, implementation is miminal and clean. Very well done.

My only question is whether the name shouldn't be import options as opposed to inputs as inputs is a little too close to input, the protected keyword.

Ps. I in fact implemented something very similar not too long ago that was private until just now. :) - https://github.com/mottosso/qargparse.py

davidlatwe commented 4 years ago

My only question is whether the name shouldn't be import options as opposed to inputs as inputs is a little too close to input, the protected keyword.

I did think of just named it options, but potentially it could be used in other place then option box, so I thought maybe inputs is a better fit. Or maybe controllers ? Or qparse for shorter name ! Hehe, nice project and thanks for sharing ! @mottosso

I found that the action widget's label got an ugly drop shadow when it's been hovered in Nuke.

Could not find any reason or solution but this https://stackoverflow.com/q/52838690/4145300.

So it looks like some how Avalon's style has lost or incomplete in action widget so the Nuke's default style pops up, not sure. :/

Anyway, I have added a commit to fix this. :relaxed:

BigRoy commented 4 years ago

Sorry for the late reply. First off, this looks very nice! Very good work. I did have a question and a note.

One more abstraction layer, allow to define your own QWidget completely

Should we maybe add a single more abstraction layer so that if one wanted, they could provide their own Widget completely as Option dialog as opposed to these "predefined option widgets". Overall I like the fact that, with a bit of documentation, we seem to be able to make it easy for anyone with little understanding of programming to produce these widgets. However, some might want more control over the full feel of the widget. So I was thinking, what if we did it like this:

Instead of passing the options =[inputs.Bool(), inputs.Int()] to the loader directly we would pass it an OptionsWidget, like so:

# psuedocode

class OptionsWidget(inputs.OptionsWidget):
    inputs = [
        inputs.Bool("iAmHuman", help="Are you a human ?"),
        inputs.Int("age", default=15, min=0, help="How old are you ?"),
        inputs.Float("height", default=170, help="How tall are you ?"),
        inputs.Double3("position", help="You're location cord."),
        inputs.GetOne("sex", ["Male", "Female"], as_string=True),
        inputs.String("message",
                      placeholder="Say something",
                      help="Leave a message."),
    ]

class Loader(api.Loader):
    options = OptionsWidget

    # Etcetera

So that if someone wanted to attach fully their own widget they could inherit from OptionsWidgets (or even just an OptionsBase? which only requires to have a function options() to be available to retrieve the chosen options in the end. That would also allow e.g. a UI that has already been built by someone to be used.

Plus it would also allow to re-use a single OptionsWidget across mutiple loaders if you wanted to. Especially if somehow the particular Loader and Representation would also be passed along to the OptionsWidget so that even more customization could be done for the options if one wanted (like if only specific metadata is present on a specific subset/version/representation).

E.g.

class BaseOptionsWidgets(QtWidgets.QWidget):
    def initialize(loader, representation):
        # Implement this in subclass if you want to do customization
        # for specific loaders or representations and you build your own
        # widget
        pass

    def options():
        # Return the output values of your options dialog
        raise NotImplementedError("Must be implemented by subclass.")

And we would have the Widget you have now as a Helper widget to make it easy to quickly throw together some options if you don't want to write full Qt Widgets.

What do you all think?

Styling/spacing

Is there any particular reason the spacing seems to be so big in the preview screenshot? There seems to be an awfully lot of space between the inputs. Or is that just my personal taste that's different than yours? I wondered if there was any particular reason for it.

@davidlatwe you're on fire again! Lovely work. 🚀

tokejepsen commented 4 years ago

What do you all think?

I like it!

mottosso commented 4 years ago

Should we maybe add a single more abstraction layer

Eeek! Less abstractions = better.

I didn't get what the problem this would solve? KISS, resist the urge to overengineer. I would suggest putting that on the shelf until there's an actual problem or usecase it could be used to solve; although ideally we wouldn't fit a solution to a problem, but rather figure out a solution starting from the problem.

BigRoy commented 4 years ago

I didn't get what the problem this would solve? KISS, resist the urge to overengineer. I would suggest putting that on the shelf until there's an actual problem or usecase it could be used to solve; although ideally we wouldn't fit a solution to a problem, but rather figure out a solution starting from the problem.

Totally see what you mean, and I agree. Preferably the implementations gives the barebones of what everyone needs and allows for the studio configurability we all want.

In that case I'd simplify this and allow to pass solely a QWidget and move the custom helper classes for inputs into its own repository, like qargparse. Personally I feel that it is somewhat overengineering functionality that is trivial to put in a QWidget already. Less code, less to maintain in core and the same result. For developers familiar with Qt they now also need to familiarize them with this "Inputs" API which is redundantly limiting the options for the developer. It sounds blunt in words and I definitey don't mean it that way, it looks very nice. I just wondered why we lock it in this much when it's redundant to do so? The code works perfectly fine without it and is simpler to maintain.

Maybe saying "more abstraction" for what I tried to propose described it wrong.

mottosso commented 4 years ago

Not sure what to make of this to be honest. The current code solve a specific issue, and nothing more. That's how I would argue code should get written. Keeping it limited in terms of an API makes it more predictable what people do with it and enable us to build off of those assumptions, much like how Avalon "limits" the user in what they are able to do in terms of Python or Qt. We let users add any widget, and suddenly we'll have request about why their media player isn't contacting Amazon Cloud properly when the window is minimized, because they need their deep learning web camera tracking software to toggle some option on/off.

I think your point is too theoretical. xD It's open source and Python so, if someone familiar with Python wanted to customise things further, they already could.

davidlatwe commented 4 years ago

Personally I feel that it is somewhat overengineering functionality that is trivial to put in a QWidget already.

I think the main convenience was when retrieving the user inputs, but I didn't really comparing both ways. Just my guts feeling :grimacing:

What about this :point_down:

if isinstance(loader.options, QtWidgets.QWidget):
    dialog = loader.options(parent=self)
else:
    dialog = OptionDialog(parent=self)
    dialog.setWindowTitle(action.label + " Options")
    dialog.create(loader.options)

if not dialog.exec_():
    return

# Get option
options = dialog.parse()

So if anyone prefer or need to use self-implemented dialog, just plug it into Loader.options, as long as the method parse has implemented. :relaxed:

Also, I have put qargparse into avalon.vendor, and tools.inputs was removed, changes (just a little) will be pushed soon.

@BigRoy

Is there any particular reason the spacing seems to be so big in the preview screenshot?

Here is the result after adopting qargparse, no more redundant spacing !

BigRoy commented 4 years ago

I think the main convenience was when retrieving the user inputs, but I didn't really comparing both ways

Do you mean for the code in core? Or a developer designing the options interface?

Anyway, I feel this extra if/else statement is more confusing things than streamlining the API for the Options on the loader. If no one feels anything for being able to define any custom Widget I'd be happy to drop this conversation and pick it up later. Still feel it's slightly odd design but nothing that I can't get past. :)

However, if we later ever want to push this in cleanly it's only possible with this if/else statement to make sure loaders remain backwards compatible.

davidlatwe commented 4 years ago

However, if we later ever want to push this in cleanly it's only possible with this if/else statement to make sure loaders remain backwards compatible.

Yeah, I was thinking about that too! But then I thought adding an if/else statement wasn't a big issue for remaining backward compatible in this case, so I've decided to stick to the current implementation for this PR. :)

Will merge this tomorrow if no other objections 🚀 Thank you all !

Edit:

Do you mean for the code in core? Or a developer designing the options interface?

I meant for a developer designing the options interface. :)

davidlatwe commented 4 years ago

Sorry, just spotted another issue while looking at @iLLiCiTiT's proposal in above review comment.

Turns out the action highlighting will stop changing while mouse pressed and hold, see GIF below :point_down:

mouse_pressed_unfixed

It look's like the widget region of each action for enterEvent and leaveEvent has become entire menu after mouse is pressed.

After hours of googling and messing around, finally realized that the menu isn't a list of widgets but just pointers, so after mouse pressed, those enterEvent and leaveEvent implemented on each action widget has been ignored (by the menu's mousePressedEvent).

So instead of re-implementing action widget's enterEvent and leaveEvent, I add a subclass of QMenu with mouseReleaseEvent, mouseMoveEvent and leaveEvent re-implemented.

Now our menu will keep highlighting active action no matter mouse was pressed or not. :point_down:

mouse_pressed_fixed

Okay, I think this PR is really good to go :relaxed:

mottosso commented 4 years ago

Looking good! :D

davidlatwe commented 4 years ago

Merge !

getavalon / core