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

Add scripting for custom property types #3971

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

dogboydog
Copy link
Contributor

@dogboydog dogboydog commented Jun 16, 2024

Aims to fix #3419

Related to #2902

Demo of the functionality so far in the console:

> for (let t of tiled.project.propertyTypes) { tiled.log( t.name) }
Breakable
Door
Enemy
KillZone
> tiled.project.findTypeByName('Enemy').name
$0 = Enemy
> tiled.project.findTypeByName('Enemy').color
$1 = #ff0000
> tiled.project.findTypeByName('Enemy').isClass
$2 = true 
> tiled.project.findTypeByName('Enemy').usageFlags  & ClassPropertyType.LayerClass
$2 = 0
> tiled.project.findTypeByName('Enemy').usageFlags  & ClassPropertyType.MapObjectClass
$3 = 4

TODO:

  • Editing properties on custom types
  • Adding new enum and class types to the project
  • Adding new properties to class types, including properties that are classes
  • Removing types from the project
  • Add scripting doc

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 great start, @dogboydog! There are some open questions, especially what to do with the raw pointer and potentially whether we should rather return class members as an array (though that does seem a little clumsy and somewhat pointless since internally it's still a map for now).

QColor color() const { return mClassType->color; }
// TODO: " No viable overloaded '=' "
// void setColor(QColor &value) { mClassType->color = value; }
QVariantMap members() const {return mClassType->members; }
Copy link
Member

Choose a reason for hiding this comment

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

There's a planned feature that would enable manual ordering of class members, so it might be good to expose this as an array instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure whether to make it an array in advance of that feature, but I did want to note that accessing the members through this property doesn't seem to allow adding new members and doesn't handle members of a class type (at least the last time I tried before your push)

Copy link
Member

@bjorn bjorn Jun 19, 2024

Choose a reason for hiding this comment

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

What do you mean with "doesn't handle members of a class type"? For modification we'll need setMembers or addMember, because indeed the members are returned by value here (effectively a copy).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd have to test again, but I think when I tested, if a member was a class, it just came back as QVariant and I couldn't do anything with it like get its name or its own nested members

Copy link
Member

@bjorn bjorn Jun 19, 2024

Choose a reason for hiding this comment

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

Hmm, you should get a PropertyValue, so it should at least have exposed its properties like typeName and value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll test again when I get a chance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it's working for properties that are classes 👍

> tiled.project.findTypeByName('Class12').members['myChild'].value['hello']
$9 = false

Thanks for fixing the removal of types too . I wonder if removeTypeByName should return a boolean reflecting whether or not the type was found


QColor color() const { return mClassType->color; }
// TODO: " No viable overloaded '=' "
// void setColor(QColor &value) { mClassType->color = value; }
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is just cause of passing by reference. The argument should either be by value or by const-reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither of those compile right now.

void setColor(QColor value) { mClassType->color = value; }

C:\Users\chris\tiled\src\tiled\scriptpropertytype.h:116: error: C2678: binary '=': no operator found which takes a left-hand operand of type 'const QColor' (or there is no acceptable conversion)

C:\Users\chris\tiled\src\tiled\scriptpropertytype.h(116): note: Conversion loses qualifiers

Or
void setColor(const QColor &value) { mClassType->color = value; }

C:\Users\chris\tiled\src\tiled\scriptpropertytype.h:116: error: C2678: binary '=': no operator found which takes a left-hand operand of type 'const QColor' (or there is no acceptable conversion)
C:\Users\chris\tiled\src\tiled\scriptpropertytype.h(116): note: Conversion loses qualifiers

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it's just because mClassType is a const ClassPropertyType*, so you can't modify it. You'd need a non-const ClassPropertyType*.

QStringList values() const { return mEnumType->values; }

private:
const EnumPropertyType *mEnumType;
Copy link
Member

Choose a reason for hiding this comment

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

A raw pointer is problematic here, cause there is nothing that makes sure the ScriptEnumPropertyType instance is deleted when the EnumPropertyType instance is deleted. There's a few potential solutions we can try:

  • Referring to types using shared pointers, so that the wrapper object can keep the type alive.
  • Adding a pointer to the ScriptEnumPropertyType from the EnumPropertyType, so that it can delete the wrapper upon destruction.
  • Making a copy of the type that is owned by this wrapper. Though that means changes won't automatically apply, but we'd instead need to have a Project.addType or Project.setType to assign a modified object back to the project.
  • Remembering just the name of the type and looking it up each time it is accessed (might get confusing in case the type is renamed).

I'm not sure at the moment which will be the best way forward. It depends also on the expected usage pattern of the API.

Copy link
Contributor Author

@dogboydog dogboydog Jun 18, 2024

Choose a reason for hiding this comment

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

To me the first two seem like the best options. I'd prefer to retain the ability to modify a type's fields just by setting them -- I think that's simpler. So that would mean no option 3. Option 4 seems a bit messy for the reasons you mentioned (we may also want to allow renaming the types from scripting I would think, so that would be complicated by option 4)

For option 2, I guess we would have to check if there already has been a ScriptPropertyEnumType created for an EnumPropertyType before we would create a new one. Also if the main types would hold a pointer to the script wrapper, maybe they should handle creating the script wrapper themselves?

Option 1 also sounds promising but challenging for my skill set . Maybe with some pointers (ha) I could do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if we go for the shared pointer route, I wonder where the code for creating the shared pointers would go, maybe in PropertyTypes ? If we get that working, it would solve the const pointer issue too and allow the setters for color, drawFill, etc. to work

@dogboydog
Copy link
Contributor Author

I rebased this from master and tried to take another look, but I still don't feel confident in how to proceed. PropertyTypes returns const pointers which I can't use to modify values nor to try to store a pointer back to the script type to manage cleaning up. Also, you mentioned that there may be a change in how property types are stored coming up, so that they aren't sorted by name and rather allow the user to re-order them, so if I were to try to change PropertyTypes now I'm not sure if that would conflict.

Sorry, I don't write much C++ and so I'm not used to dealing with pointers/const qualifiers

@bjorn
Copy link
Member

bjorn commented Sep 10, 2024

@dogboydog That's alright, thank you for rebasing the PR!

I would say not to worry about introducing conflicts, since I'd anyway be around to help resolve them again. But yeah, I understand the ownership and const stuff can be a little much if you're not used to C++. I'll see if I can have another look this week.

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.

Scripting: the API for getting PropertyTypes
2 participants