-
Notifications
You must be signed in to change notification settings - Fork 964
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
Allow to create variants of custom objects #7403
base: master
Are you sure you want to change the base?
Conversation
c9fc931
to
5841389
Compare
16ff4ad
to
eccdb98
Compare
ab9a9bf
to
897e8ca
Compare
newIDE/app/src/ObjectsRendering/Renderers/RenderedCustomObjectInstance.js
Outdated
Show resolved
Hide resolved
@@ -8,7 +8,7 @@ const gd: libGDevelop = global.gd; | |||
* and unserializeFrom method. | |||
* | |||
* @param {*} serializable | |||
* @param {*} methodName The name of the serialization method. "unserializeFrom" by default | |||
* @param {*} methodName The name of the serialization method. "serializeTo" by default | |||
*/ |
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.
Probably worth removing the comment and add a better typing of the argument. 'serializeTo' | ...
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'm not sure what to do because we probably don't want (or will forget) to update the type when a new function is needed in Core. I guess it's fine to leave it as it is.
@@ -2196,6 +2254,7 @@ export default class SceneEditor extends React.Component<Props, State> { | |||
hotReloadPreviewButtonProps={ | |||
this.props.hotReloadPreviewButtonProps | |||
} | |||
isListLocked={true} |
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.
Can you confirm this is fine and that we don't need additional changes to unblock some authorised changes to the variables?
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 instance variable editor allows to reset back an instance variable to the default value. I guess it allows to remove undeclared instance variables.
There is probably something to do to help users understand why the "add" button is disabled and how they can add an object variable, but I guess this is already an improvement as it now forbids to add an undeclared instance variable which could be miss-understood as adding an object variable and sound like a bug when the events give no auto-completion for it.
return closeTabsExceptIf(state, editorTab => { | ||
const editor = editorTab.editorRef; | ||
if (editor instanceof CustomObjectEditorContainer) { | ||
return ( |
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.
You're sure about this predicate?
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.
Variants tabs are CustomObjectEditorContainer
with the variant name set (as they works mostly the same as the default variant tab).
@@ -2146,6 +2197,11 @@ export default class SceneEditor extends React.Component<Props, State> { | |||
objectGroup | |||
); | |||
} | |||
// TODO Set the `global` attribute correctly. |
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.
Can we either do this or not pass global: true/false at all if we don't know or if not useful?
const customObjectConfiguration = gd.asCustomObjectConfiguration( | ||
object.getConfiguration() | ||
); | ||
if (customObjectConfiguration.getVariantName()) { |
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.
Is there a risk we use default variant and so this is not detected?
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, an empty name should be forbidden. I made it falls back on the asset name.
Changes
Known issues
Demo
Require