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

Early PR for Custom/Enum Props #2941

Merged
merged 17 commits into from
Jun 29, 2021
Merged

Early PR for Custom/Enum Props #2941

merged 17 commits into from
Jun 29, 2021

Conversation

svipal
Copy link
Contributor

@svipal svipal commented Nov 28, 2020

resolves #1211
Hello ! I've started to implement enum props, as discussed in the discord and on the linked issue.

I made an early PR even though not everything is implemented, as suggested by bjorn.
Here's what's not yet in it :

  • enum props are the only possible custom props instead of just one subclass
  • properties of custom type do not persist between sessions

Here's what works :

  • loading/saving custom properties from project
  • editing/saving enum props with the editor
  • making new properties with thus made "enums"

Let me know what you think !

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good start! But I think there's one major issue, which is that you're storing the whole definition of the custom type with each value. I think they should be separated. We can have a "default value" member of the custom type (though I'd say we pick the first one by default for now), but the actual value should not be stored along with the type definition.

I think we need a separate struct for storing the value, something like:

struct CustomValue
{
    QVariant value;
    QString type;
};

It's going to be a bit confusing, but we would then store this in the QVariant storing a property value, and when value.userType() == qMetaTypeId<CustomValue>(), we'd be able to look up both the actual value (using value.value<CustomValue>().value) and its type. We'd use the type to look up the information about the type so that we can set up appropriate widgets in the VariantEditorFactory.

In addition:

  • The toExportValue function would need to handle custom types.

  • typeToName will need to take the full QVariant as parameter instead of just the type ID, so that it can return the CustomValue::type member when appropriate.

  • nameToType can't just return a type ID anymore. I think the function can be integrated into fromExportValue, passing the type as string instead of int to that function. Considering this, it probably makes sense to integrate typeToName also with toExportValue, probably making it return a CustomValue with the value member being the value for export and the type member the type name to export.

I've left further minor comments inline.

src/libtiled/customproperties.h Outdated Show resolved Hide resolved
src/libtiled/customproperties.h Outdated Show resolved Hide resolved
src/libtiled/customproperties.h Outdated Show resolved Hide resolved
src/libtiled/customproperties.h Outdated Show resolved Hide resolved
src/tiled/addpropertydialog.cpp Outdated Show resolved Hide resolved
src/tiled/tiled.pro Outdated Show resolved Hide resolved
src/tiled/variantpropertymanager.cpp Outdated Show resolved Hide resolved
src/tiled/variantpropertymanager.cpp Outdated Show resolved Hide resolved
src/tiled/varianteditorfactory.h Outdated Show resolved Hide resolved
src/tiled/objecttypeseditor.h Outdated Show resolved Hide resolved
@edin-m
Copy link

edin-m commented May 16, 2021

Is this being worked on?

@bjorn
Copy link
Member

bjorn commented May 17, 2021

@edin-m There hasn't been any activity for some months, so I think this change is currently abandoned. Since it's still a priority request from a sponsor I do intend to start work on it soon. Are you asking because you'd like to help complete this feature?

@edin-m
Copy link

edin-m commented May 17, 2021

Maybe... I'll see after the components feature.

@bjorn
Copy link
Member

bjorn commented Jun 7, 2021

I've rebased the changes on the latest master and applied some initial review feedback. I plan to continue working on this change with the goal of finishing it later this week.

svipal and others added 16 commits June 25, 2021 22:44
Here's what's not yet in it:

* defining custom property types other than enums
* properties of custom type do not persist between sessions

Here's what works:

* loading/saving custom properties from project
* editing/saving enum props with the editor
* making new properties with thus made "enums"
This took a bit of refactoring, such that values of a custom type are
now kept in a CustomValue instance that refer back to their type.
Internally the ID is used to identify the custom type (CustomValue
carries a typeId).
When adding, removing or changing values, the custom types are now
automatically saved. The explicit "Load" and "Save" buttons have been
removed, as well as the "Debug" button.

Also switched it from using a QTableWidget to a QTreeView and a model
derived from QStringListModel for the values.

Empty values are no longer automatically removed and new values get a
default text.
That seems more intuitive and is consistent with ObjectType.
Seems more appropriate now that "CustomType" is renamed to
"PropertyType".
This way custom types can be supported much more generally and based on
the existing features in the QtPropertyBrowser solution, requiring less
custom code to support all the various parameters for other types than
enums later on.

One downside is that now custom types no longer work in the Object Types
Editor, because it does not use the PropertyBrowser. Further refactoring
should allow sharing this widget, by taking out the handling of all the
built-in properties.
The only difference is that Object::resolvedProperty can return the
value on the current object, in addition to resolving an inherited
value, but we don't need a separate function for that currently.
Introduced CustomPropertiesHelper to share some functionality between
the PropertyBrowser and the ObjectTypesEditor, which provide two very
similar editors yet work in a rather different context.

The CustomPropertiesHelper also covers the necessary handling of
PropertyValue / PropertyType.

One remaining issue with the enums is that now when the user changes the
values of the enum, the new values are not reflected in the properties
view. Previously this worked because the QComboBox was re-created, but
now we need to reset the "enumNames" attribute for the created
properties.
This exposes a somewhat strange behavior in the QtEnumPropertyManager,
which will reset the value of the property when the list of valid names
it changed.
Change QtEnumPropertyManager::setEnumNames to not reset the current
value, but rather just keep it within range. This is somewhat less
destructive and at least allows renaming and adding enum values without
issues.
* Removed unused "color" and "valueType" members of PropertyType
* Renamed 'Object Types Editor' to 'Enums Editor'
* Added a line edit for the enum name (can still be renamed in list as
  well)
* Show text on 'Add Enum' and 'Add Value' buttons
* Use QTreeViews instead of QTableViews
* Removed unnecessary splitter
* Apply a default name to new enums
@bjorn bjorn merged commit 4cdcdb1 into mapeditor:master Jun 29, 2021
@bjorn
Copy link
Member

bjorn commented Jun 29, 2021

So in the end, we now have (stripped):

struct PropertyType
{
    int id = 0;
    QString name;
    QStringList values;
};

struct PropertyValue
{
    QVariant value;
    int typeId;
};

Where the property types (stored in the project) are for now only defining enums with their possible values, and the property value class always wraps an int value together with the ID of the enum type. This is then used by the Properties view to display the value using a combo box where the other possible values can be selected.

When saving and loading, the value is currently written as its string value alongside the name of its type. This may still change, to either replace it with the int value and type ID or to add those alongside.

Thank you @svipal for getting this change started!

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.

Request to Add Support for Enumeration Properties
3 participants