godotengine
Using memnew to create a Node that has more than one constructor parameter fails when using an initialiser list.
Godot version
v4.1.1.stable.custom_build [bd6af8e0e]
godot-cpp version
4.1 4eed2d7be017ef06521665604990114c9fff7c77
System information
Godot v4.1.1.stable (bd6af8e0e) - Linux Mint 21.2 (Victoria) - Vulkan (Forward+) - integrated Intel(R) Graphics (RPL-P) () - 13th Gen Intel(R) Core(TM) i7-1360P (16 Threads)
Issue description
When creating a ::godot::Node we should use memnew instead of new. I have made a subclass of Node3D that has two constructor parameters. I tried to create an instance with
memnew (MySubclassOfNode3D {x, y})
This fails, because memnew is a macro, and to the C++ preprocessor this is a macro call with two arguments, rather than one.
I previously reported this issue here: https://godotforums.org/d/36888-using-memnew-to-create-a-node-that-has-two-constructor-parameters
Steps to reproduce
Create a subclass of Node3D with more than one constructor parameter and create an instance with memnew.
Minimal reproduction project
N/A
Btw, if I redefine the memnew macro as
#define memnew(...) ::godot::_post_initialize(new ("") __VA_ARGS__)
then it works. But I do not know enough about macros to be able to say whether this may cause other issues.
Yes that works. See also https://godotforums.org/d/36888-using-memnew-to-create-a-node-that-has-two-constructor-parameters
Then the syntax you're using is not the preferred one here, not a bug IMO but a limitation on what syntax is allowed
I just don't think godot should unnecessary limit the subset of C++ syntax it allows ...
Don't you think it is worthwhile to see if my redefinition of the macro may work for you?
I don't know what consequences it would have, but using bracket syntax is rare and unusual IMO, parenthesis syntax is the norm
parenthesis syntax is the norm
I beg to differ.
There are other cases where curly braces may be useful, e.g. a subclass of ::godot::Node with an std::vector <int> argument initialized like so:
memnew (MySubclassOfNode {1, 2 ,3})
Does this syntax work in engine code?
One of the design goals of godot-cpp is to support (as much as possible) the same API as internally used by the engine, so that code can be copied between both contexts, or that the same code (using some minimal #ifdefs) can be compiled either as a GDExtension or as a Godot module.
So, I don't think we should support this syntax in godot-cpp, unless it is first supported in Godot itself.
We don't use the initializer style syntax anywhere in the engine code AFAIK, and the macro is identical I believe so wouldn't work
Am i correct that you are saying that my suggested modification of the macro would work fine in the godot engine code base?
No I'm not saying that at all, I'm saying that the macro in the engine is the same macro so the same limitation applies, hence why I said "wouldn't work", the macro in the engine is:
#define memnew(m_class) _post_initialize(new ("") m_class)
So I'm confirming that this won't work with the engine code either, and agreeing that this change should not be made unless also made for the engine
So how do we proceed from here then? Will someone implement the change in the engine?
So how do we proceed from here then? Will someone implement the change in the engine?
Given that this is something you want, it'd probably make the most sense for you to do that. :-) You could open a PR on the engine repo with your proposed changes to this macro, which would lead to a discussion among engine contributors/maintainers, and if approved, the change would be merged to the engine.
If it were only for me, then I'd be happy with my local redefinition of the memnew macro.
I raise the issue to make a small contribution, to make a tiny tiny little improvement that may be beneficial to godot and make godot a little more agreeable to other programmers.
Anyway, I'll have a look at how to open a PR.
I'm pretty sure that the following two generate identical assembly:
new (newArgs) T(classArgs)
new (newArgs) T {classArgs}
Because the first form, although we're asking to construct a T and pass it by value to placement new, thus invoking the copy-constructor, URVO guarantees that the T will be constructed in-place into new.
Can somebody with more experience than me please confirm this?
