-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
base: master
Are you sure you want to change the base?
Add scripting for custom property types #3971
Conversation
There was a problem hiding this 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).
src/tiled/scriptpropertytype.h
Outdated
|
||
QColor color() const { return mClassType->color; } | ||
// TODO: " No viable overloaded '=' " | ||
// void setColor(QColor &value) { mClassType->color = value; } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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*
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have QSharedPointer<ClassPropertyType> mClassType
, we can assign a new color to mClassType->color
. Just note that a UI update will need to be triggered as well and we also might want to save the project. These things currently happen only from PropertyTypesEditor::applyPropertyTypes
, which is called from all the places where changes are made to the types.
I'm not sure about triggering saving of the project immediately though. It would be better to schedule this to avoid too much file writing in case of changing a bunch of things from a script.
I think we could start with just triggering emit Preferences::instance()->propertyTypesChanged()
after applying changes.
b1486e2
to
e80e00b
Compare
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 |
@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. |
5729b4e
to
0232264
Compare
src/tiled/scriptpropertytype.h
Outdated
bool drawFill() const { return mClassType->drawFill; } | ||
// void setDrawFill(bool value) { mClassType->drawFill = value; } | ||
int usageFlags() const { return mClassType->usageFlags; } | ||
//void setUsageFlags(int value) { mClassType->setUsageFlags(value); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setter for usage flags accepts bool value
to either turn the specified flags on or off, but it doesn't in itself provide a way to just set the usage flags to the specified value. Would script writers expect to be able to just hard set the usage flags to a value (personally, this is what I think makes sense so that you can do things like &= , |= the flags) or have a method like setUsageFlags(flags, value: bool) ?
Without changing the setUsageFlags in propertytype.cpp, to achieve a setter, I could turn off all flags with setUsageFlags( ClassUsageFlag. AnyUsage , false)
and then turn the specified flags on with setUsageFlags(flagsFromScriptUser, true)
if that makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed that signature is quite UI-oriented since it's used in response to the user toggling checkboxes. I agree from a scripting API point of view, we probably just want to be able to set the flags directly and you could introduce a void ClassPropertyType::setUsageFlags(int flags)
overload for this (though actually two calls, like you suggested, is also a fine solution).
…ead-only propertie
…ypes.all instead) and add getter for class property members. getting members of class types does not work though
…s a list of types and move the utility methods for custom types to tiled.project
This way, the type is kept alive by the ScriptPropertyType, which avoids crashes when accessing roaming pointers. Also, since it now has a non-const reference we can make the properties writable.
…type from scripting
d293bea
to
4a697eb
Compare
SharedPropertyType newClassType = SharedPropertyType(new ClassPropertyType(name)); | ||
project()->propertyTypes()->add(newClassType); | ||
applyPropertyChanges(); | ||
return findTypeByName(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just do toScriptType(newClassType)
.
Maybe we could also take an optional members
argument? And similarly, a value
argument for addEnumType
?
I personally wonder a little if we couldn't instead just have the following API:
var fooType = new ClassPropertyType("Foo");
tiled.project.addType(fooType);
Finally I wonder if we shouldn't instead implement it like tiled.project.propertyTypes.add(fooType)
, but then of course propertyTypes
can't be a QVector<ScriptPropertyType *>
but would need to be something like a wrapping ScriptPropertyTypes*
.
It depends a little on whether we'd want to support types anywhere else than the project (I'm considering whether it would make sense to support types defined by scripted extensions, which are not saved in the project). Opinions welcome. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, open to other opinions. The types don't really mean anything until they're added to the project, but maybe people would prefer having a constructor like this? I guess since unloading a project will restart the script engine, there's no way you could get into trouble trying to add a type to multiple projects from one reference.
I also have to add code to prevent duplicate names from being added. Maybe that should go in propertytype.cpp and then log a script error if we fail to add one.
Everything seemed OK yesterday with the latest commit but for some reason now even just using the property types editor is raising this exception 🤔
Aims to fix #3419
Related to #2902
Demo of the functionality so far in the console:
TODO: