Open lethefrost opened 5 months ago
I'd say this makes more sense as a proposal, as there's no bug here but a limitation that could be discussed
Thank you for the reference!
But I am still not sure about why in the editor_properties.cpp uint32_t
is used now instead of uint64_t
. I also don't understand why in the pull request, the constant 32
need to be stored in int64_t
(aren't unsigned int
or even short
enough to store 32
? I don't think in shift operation it requires the shift distance to be a long int...), and why the variable value is int64_t
instead of uint64_t
?
If it doesn't take you too much time, would you mind bothering explaining a little more details? It'd be really appreciated.
I'm not sure about the reasons, but my guess is:
int
is 64-bit, many parts of the engine use 32-bit (Vector2i
, PackedInt32Array
, etc.). Also note that 3.x only has PoolIntArray
(not 32 and 64), so this may be somewhat legacy.uint64_t
should not be used since int
is signed and this may lead to problems with conversions to/from unsigned. Most likely, 32-bit was chosen for reasons of portability and reducing potential bugs.I'm not sure about the reasons, but my guess is:
- Even though
int
is 64-bit, many parts of the engine use 32-bit (Vector2i
,PackedInt32Array
, etc.). Also note that 3.x only hasPoolIntArray
(not 32 and 64), so this may be somewhat legacy.uint64_t
should not be used sinceint
is signed and this may lead to problems with conversions to/from unsigned. Most likely, 32-bit was chosen for reasons of portability and reducing potential bugs.- In practice, you are unlikely to encounter problems as a result of this limitation. 32 flags is quite a lot to clutter up the inspector. If you need 33+ flags, then there is a high chance that you could soon exceed the 64 flag limit too. In any case, you will probably need an array or a custom inspector plugin to display the property in a compact view.
Yeah, I totally agree with point 3. I was just trying to figure out something that doesn't quite make sense to me, but it shouldn't have a problem because in most scenarios nobody would go over that limit. Thank you so much for take the time to help and explain!
I also have a little more questions about bitfields, if you don't mind taking a look at them as well.
In gdscript, I didn't quite find some helpful bit operations/helper functions like popcount in C#/C++, or just simply toggle, set, clear, check (all/any) bits or group of bits, which make it easier and handy for developers to manipulate and utilize the flags in their game logics.
Currently, in order to use exported checkboxes in inspector, it has to be an @export_flags
bit field, but I'd say the lack of handy tools would detract from its usability. I believe wrapping the widely reused flag operations in named functions can make the code more human-readable, and appear less intimidating to beginner developers who are learning gdscript because of its friendliness.
Another inconvenience is the flag types are only defined in the string arguments of @export_flags
annotation, so there's no way to reuse them elsewhere. In order to make use of the bit field member, developers always need to maintain another enum or dict storing corresponding flags values, which takes up unnecessary efforts. I am wondering if @export_flags
can just take one enum/dict argument then users only need to maintain one, and it can just work like @export
vars with an enum type hint.
I've been seeing many proposals (https://github.com/godotengine/godot/issues/74418 , https://github.com/godotengine/godot/issues/71234#issuecomment-1430763393, https://github.com/godotengine/godot-proposals/issues/923) regarding improving the usability of the bit fields.
I am wondering if this area is open for contribution now, or do you prefer it to maintain its current behavior? If I make a pull request, would it be likely to be accepted? (Haven't contributed to godot yet, and I saw the best practices instruction says "before attempting to contribute, it is important to discuss the actual problems with the other developers or contributors")
I am planning to add:
int
type can have methods like Strings do, if it does, then adding those methods to int
could be preferred). @bitfield
annotation for enums as https://github.com/godotengine/godot-proposals/issues/923#issuecomment-1276774846 suggested. However, since the integers in enum are just 2's powers, and only represent 1 set bit at each position, I think maybe @bitflags
could be a better annotation name than @bitfield
.@export_flags
to take one enum argument and extract the name strings for each flag checkbox to display in the inspector, just like the @export
var with enum type hint dropdown menu doesDoes it sound okay to you?
Thank you so much again for your time and patience!
a
@bitfield
annotation for enums
This is a fairly cosmetic change, it just changes "enum" to "flags" in the documentation. We could add validation for values of enum marked with @bitfield
, but I'm not sure that's a good idea since combining flags is fine.
I am wondering if this area is open for contribution now, or do you prefer it to maintain its current behavior? If I make a pull request, would it be likely to be accepted?
I was thinking that we could add a BITFIELD
kind to the GDScriptParser::DataType
and add the BitField[TEnum]
pseudo-type. Note that checks exist only in the static analyzer; at runtime it is still an int
, as in the case of enums.
The problem is that we can't add methods like set_flag()
, has_flag()
, etc. without some kind of runtime wrapper (or add these methods to int
as you suggested). We can't resolve this only in the static analyzer because GDScript supports dynamic typing and type-unsafe operations only cause warnings, not compile-time errors.
But even though there are no BitField
methods, you could still use bitwise operators. This would improve type inference, for example now <MyEnum expr> | <MyEnum expr>
is reduced to int
, and in the future we may start inferring it as BitField[MyEnum]
.
But I think we need some refactoring of GDScript's static analysis before adding major new features (adding a new DataType
kind is probably a pretty significant change).
allow
@export_flags
to take one enum argument and extract the name strings for each flag checkbox to display in the inspector, just like the@export
var with enum type hint dropdown menu does
Personally, I don't like the idea of annotation overloads, even though they are resolved at compile time so they don't cause dispatch problems at runtime, unlike method overloads do. But we could add spread syntax instead, see below. Also, we could add bitfields support for the bare @export
annotation, similar to how it supports global, native, and GDScript enums. @export_enum
and @export_flags
are for custom enum/flag lists only, in my opinion.
@export var a: MyEnum
@export var b: BitField[MyEnum]
Another inconvenience is the flag types are only defined in the string arguments of
@export_flags
annotation, so there's no way to reuse them elsewhere.
An alternative approach is to add spread syntax, see godotengine/godot-proposals#8439. Annotations support constant expressions. You could declare a constant array and reuse it like this:
const FLAGS = ["A", "B", "C"]
@export_flags(...FLAGS) var x: int
@export_flags(...FLAGS) var y: int
I started working on the second part of #82808 and it's already partially done in my local branch, but I've paused it for now.
Hi Danil! Sorry for the late response, and thank you for providing those helpful information and sharing your thoughts!
a
@bitfield
annotation for enumsThis is a fairly cosmetic change, it just changes "enum" to "flags" in the documentation. We could add validation for values of enum marked with
@bitfield
, but I'm not sure that's a good idea since combining flags is fine.
I think what I was suggesting is a little more than just turn the displayed type name into flags
in the documentation, but also change the values' default initializing strategy in enum
s.
# Without annotations, the original default values are
enum PlainEnum{
A = 0,
B = 1,
C = 2,
D = 3
}
# But with the annotation, the default values would automatically be generated as powers of 2
@bitflags
enum AnnotatedEnum{
A = 1 << 0, # which is 1 or 0b0001
B = 1 << 1, # which is 2 or 0b0010
C = 1 << 2, # which is 4 or 0b0100
D = 1 << 3 # which is 8 or 0b1000
}
And just by the way, I hope to discuss a little more about the naming:
Although I understand that these terms may often be interchangeably used by programmers in development due to their similar expressions (for instance, they all represent integer data), after reviewing relevant materials, I've confirmed that they typically represent the following concepts with some subtle differences. I believe ensuring that we use the correct terminology when designing keywords is quite important, so the future users won't feel confused or mixed up:
To give a more intuitive sense of those terms, see the example below (This is written in syntax supported in current GDScript):
class_name Spell
# These const ints are bit flags
enum ATTACK_TYPE{
FIRE = 1, # 0b0001
WATER = 2, # 0b0010
EARTH = 4, # 0b0100
WIND = 8 # 0b1000
}
# This member property is bit field
@export_flags("Fire", "Water", "Earth", "Wind") var spell_elements := 0:
# Suppose you want to add some constraints to the bitfield by setter.
set(value):
# Checks if Fire and Water is set simultaneously,
# so 0b0011 is bit mask
var bitmask := ATTACK_TYPE.FIRE | ATTACK_TYPE.WATER # 0b0011
if bitmask & value == bitmask:
push_warning("Incompatible elements coexist: Fire and Water")
else:
spell_elements = value
Therefore, if we add a new annotation to enum
s, I think @bitflags
can make more sense than @bitfield
as you or the original poster @Calinou suggested.
I was thinking that we could add a
BITFIELD
kind to theGDScriptParser::DataType
and add theBitField[TEnum]
pseudo-type. Note that checks exist only in the static analyzer; at runtime it is still anint
, as in the case of enums.The problem is that we can't add methods like
set_flag()
,has_flag()
, etc. without some kind of runtime wrapper (or add these methods toint
as you suggested). We can't resolve this only in the static analyzer because GDScript supports dynamic typing and type-unsafe operations only cause warnings, not compile-time errors.
It sounds like a good idea to me. Can you explain what the difficulties are? If we introduce a new BitField
type, which is essentially an unsigned int
(as in the previous post you confirmed it is 32-bit long) but the wrapper provides some extra methods. What was the reason that you said we cannot add methods? Is it because that a runtime wrapper is not best practice or could consume unnecessary performance? I'd appreciate it if you could tell me more details.
But even though there are no
BitField
methods, you could still use bitwise operators. This would improve type inference, for example now<MyEnum expr> | <MyEnum expr>
is reduced toint
, and in the future we may start inferring it asBitField[MyEnum]
.But I think we need some refactoring of GDScript's static analysis before adding major new features (adding a new
DataType
kind is probably a pretty significant change).
Or do we add those methods to int
? I am not sure if the int
type can have methods as other types or classes do, because I saw what are currently defined on int
are all operators.
Personally, I don't like the idea of annotation overloads, even though they are resolved at compile time so they don't cause dispatch problems at runtime, unlike method overloads do. But we could add spread syntax instead, see below. Also, we could add bitfields support for the bare
@export
annotation, similar to how it supports global, native, and GDScript enums.@export_enum
and@export_flags
are for custom enum/flag lists only, in my opinion.@export var a: MyEnum @export var b: BitField[MyEnum]
Another inconvenience is the flag types are only defined in the string arguments of
@export_flags
annotation, so there's no way to reuse them elsewhere.An alternative approach is to add spread syntax, see godotengine/godot-proposals#8439. Annotations support constant expressions. You could declare a constant array and reuse it like this:
const FLAGS = ["A", "B", "C"] @export_flags(...FLAGS) var x: int @export_flags(...FLAGS) var y: int
I think @export var b: BitField[MyEnum]
can be nice, it will be able to find the correct bit flags (suppose MyEnum
is annotated as @bitflags
) as the checkboxes' names displayed in the inspector. But it depends on if we successfully add the new BitField
type and also the BitField[FlagEnum]
pseudo type.
I have been watching and looking forward to a spread syntax thing so we could finally have variadic functions. However, I am not sure if the spread syntax here (in the exported bitfield context) is a good idea:
…
is more like ECMAScript or Swift/PHP style, but GDScript style as far as I know was designed based on Python syntax. Why not using Python/Ruby-like *
unpacking operator? It could help maintain the consistence of code style.@export_flags(…FLAGS) var x: int
and y
appear to reuse the const
string array, and avoid having to maintain many duplicates of the strings. However, it's not quite helpful when it comes to bit operators or increasing code readability for humans.Regarding point 2, consider this example:
const FLAGS = ["FIRE", "WATER", "EARTH", "WIND"]
@export_flags(…FLAGS) var spell_elements: int = 0
Then, when you want to utilize the flags, checking, toggling, setting, or clearing any combination of the flags, you either need to:
Still maintain another enum
, Dictionary
, just exactly like before, in order to make your code involving these flags make some sense to you:
enum ATTACK_TYPE{
FIRE = 1, # 0b0001
WATER = 2, # 0b0010
EARTH = 4, # 0b0100
WIND = 8 # 0b1000
}
# So you can have code with flags name that make sense
func toggle_earth() -> void:
spell_elements ^= ATTACK_TYPE.EARTH
func set_wind() -> void:
spell_elements |= ATTACK_TYPE.WIND
func check_fire_water() -> bool:
var bitmask := ATTACK_TYPE.FIRE | ATTACK_TYPE.WATER
return bitmask & spell_elements == bitmask
So using the const Array[String]
FLAGS
just saves no effort from maintaining two collections of flags' name. (Though it saves some effort when you have multiple exported properties with the same flags)
Or, just let the code look like nonsense, because you can't really get bitflags information from the String Array (Unless you do search into that array every time you encounter a flag's name, but it isn't worth the consumption of traversing and comparing).
# So your code will make little sense to human readers
func toggle_earth() -> void:
spell_elements ^= 4 # what is 4?
func set_wind() -> void:
spell_elements |= 8 # what is 8?
func check_fire_water() -> bool:
var bitmask := 3 # what is 3?
return bitmask & spell_elements == bitmask
I started working on the second part of #82808 and it's already partially done in my local branch, but I've paused it for now.
Sounds cool! Thank you for implementing this - But what stopped you?
So to wrap up here, over the discussions and potential solutions I've been seeing and pondering on this topic, I personally & currently think it would be the best, to:
@bitflags
annotation to enum
types, which will:
flags
;BitField
(which is a wrapper of int
) or not,
BitField
added:
@export var spell_elements: BitField[ATTACK_TYPE]
to export checkbox group to inspector;BitField
BitField
not added:
@export_flags(ATTACK_TYPE) var spell_elements: int
overload or replace the original variadic string parameters;int
I hope my statements make some sense. Let me know!
Thank you.
I think what I was suggesting is a little more than just turn the displayed type name into
flags
in the documentation, but also change the values' default initializing strategy inenum
s.
I'm not strictly against this, but I have some concerns:
enum MyEnum {A, B, C = 5, D, E}
is equivalent to enum MyEnum {A = 0, B = 1, C = 5, D = 6, E = 7}
. In the case of @bitfield enum
it is ambiguous: should the next value be the next power of two, use the index of the value, disable the auto-continue feature, or something else? Especially considering that @bitfield enum
values do not have to be a power of two, but can be a combination of several flags or 0 (NONE
value). Note that @export_flags
has the same issue (docs):
Note: Unlike
@export_enum
, the previous explicit value is not taken into account. In the following example, A is 16, B is 2, C is 4.@export_flags("A:16", "B", "C") var x
int
, which doesn't give you any runtime guarantees, only static checks if possible. This may seem unrelated to the proposal, but I feel it is an obstacle, we should not give false guarantees or hints about them. Changing the enumeration's auto-increment function doesn't in itself mean any guarantees, but the user may have the impression that there are some guarantees. This is a rather controversial argument, but I think it's worth considering if the feature doesn't provide vital functionality and is just syntactic sugar (see p. 3).1 << index
). This is more explicit and doesn't bloat the code. Yes, it's a bit of a boilerplate, but it's inevitable. To me it looks harmless or almost harmless. You might argue that there is a possibility of a typo, but this is also possible with implicit values. Boilerplate is bad when it spreads across many places in the codebase, but here it is localized within the enum declaration. While a large amount of syntactic sugar in a language creates other problems.@onready
, @rpc
). However, not every modifier is an annotation; there is a precedent for static
. And now we're debating whether abstract
, private
, etc. should be keywords or annotations. The same applies to @bitfield
.And just by the way, I hope to discuss a little more about the naming
Therefore, if we add a new annotation to
enum
s, I think@bitflags
can make more sense than@bitfield
as you or the original poster @Calinou suggested.
I understand your concerns and partly agree with them. But, sorry, I won't comment on this in detail to save time. We can discuss naming issues if/when we start implementing the feature. Just to note that the proposed terminology is probably borrowed from the core (VARIANT_BITFIELD_CAST
macro, is_bitfield
in the docs). Semantic meaning is important, but so is naming consistency, even if it partially distorts the meaning.
Can you explain what the difficulties are? If we introduce a new
BitField
type, which is essentially anunsigned int
(as in the previous post you confirmed it is 32-bit long) but the wrapper provides some extra methods. What was the reason that you said we cannot add methods? Is it because that a runtime wrapper is not best practice or could consume unnecessary performance?
Any value in GDScript is a Variant
(even if statically typed). GDScript is built on Variant
and some other core systems. There are actually not many features in the language that are not supported in the notional "Godot API type system". Operators, method calls, documentation, and more - all this is core stuff on which GDScript depends. It is preferable to change many things in the core rather than trying to add an alternative system to GDScript.
We have GDScriptNativeClass
wrapper that partially emulates the first class types and a number of unresolved issues related to GDScriptNativeClass
. We have unexposed GDScriptFunctionState
for await
, you can't (or rather shouldn't) obtain it, but we had a few bugs in the VM when it didn't work correctly. In theory, we could add other wrappers, but that looks like a bad idea to me. This would complicate the analyzer and the VM, since they would have to check the compatibility of int
and BitField[T]
wrapper. It would also complicate cross-language communication. For example, C# might not be aware of the GDScript wrapper BitField[T]
, so we would have to somehow sometimes convert it to int
. Given that the wrapper can be converted to int
, the rationality of the wrapper is lost. Finally, it would have a big performance impact.
Or do we add those methods to
int
? I am not sure if theint
type can have methods as other types or classes do, because I saw what are currently defined onint
are all operators.
It's possible, here's a proof of concept (core/variant/variant_call.cpp
):
static void func_int_incremented(Variant *v, const Variant **p_args, int p_argcount, Variant &r_ret, Callable::CallError &r_error) {
int64_t *n = VariantGetInternalPtr<int64_t>::get_ptr(v);
r_ret = *n + 1;
}
bind_custom(int, incremented, _VariantCall::func_int_incremented, true, int);
But the problem I see is that methods of types that are passed by value should be immutable, like in Vector2
(so instead of set_flag()
we can only have something like with_flag()
). We have a number of issues in 3.x where this condition is not met. For example, when calling a mutable method on an array element passed by value, it is actually called on a copy that is in a temporary slot, and the array element doesn't change.
The spread operator
…
is more like ECMAScript or Swift/PHP style, but GDScript style as far as I know was designed based on Python syntax. Why not using Python/Ruby-like*
unpacking operator? It could help maintain the consistence of code style.
GDScript is not based on Python and is not required to follow its syntax. The asterisk is more like pointers, the ellipsis is more obvious, so I think consistency with C++, JavaScript, PHP, and Godot documentation makes more sense here than with Python. See https://github.com/godotengine/godot-proposals/issues/1034#issuecomment-643704551 and and further discussion.
- So using the const
Array[String]
FLAGS
just saves no effort from maintaining two collections of flags' name. (Though it saves some effort when you have multiple exported properties with the same flags)
Yes, the main benefit is that you don't have to duplicate it when declaring multiple variables (like we have to in the core). As I said above, I think @export_enum
and @export_flags
are for anonymous or one-time enums/flags, plus it's useful if you want to serialize a property as a string (string keys). If you want to use GDScript enums, then you should use @export
instead. Therefore, I consider the BitField[T]
pseudotype necessary for parity with the "Godot API type system".
Thank you for implementing this - But what stopped you?
I'm postponing this due to higher priority issues and because this is quite a complex task in terms of static analysis (plus I have a few refactoring ideas that would simplify the task in the future).
Enumerations have an auto-continue feature. For example,
enum MyEnum {A, B, C = 5, D, E}
is equivalent toenum MyEnum {A = 0, B = 1, C = 5, D = 6, E = 7}
. In the case of@bitfield enum
it is ambiguous: should the next value be the next power of two, use the index of the value, disable the auto-continue feature, or something else? Especially considering that@bitfield enum
values do not have to be a power of two, but can be a combination of several flags or 0 (NONE
value). Note that@export_flags
has the same issue (docs):Note: Unlike
@export_enum
, the previous explicit value is not taken into account. In the following example, A is 16, B is 2, C is 4.@export_flags ("A: 16", "B", "C") var x
Yeah I also saw this before in the documentation. Why not we just keep the behavior we currently have for @export_flags
? I didn't realize it was a stopgap (as you described it as an "issue"), I thought it's some intended design due to some reasons I didn't know. If we already have this in the engine, and it hasn't yet cause problems, then leaving it be might not be a bad idea. At least if we add the annotation that has the same initializing strategy as @export_flags
has, it's not going to make things worse. We could just have it for now, and if later we figure out some better strategies, just replace and update it.
- Enumerations in GDScript are similar to C/C++. At runtime they are just
int
, which doesn't give you any runtime guarantees, only static checks if possible. This may seem unrelated to the proposal, but I feel it is an obstacle, we should not give false guarantees or hints about them. Changing the enumeration's auto-increment function doesn't in itself mean any guarantees, but the user may have the impression that there are some guarantees. This is a rather controversial argument, but I think it's worth considering if the feature doesn't provide vital functionality and is just syntactic sugar (see p. 3).
I did just want to add it as syntactic sugar. I haven't quite thought much about the guarantees or hints implied by enumerations, and I am not sure if I fully understand what you mean. Can you explain a little more details like what kind of runtime guarantees a user might expect? Will, in this case, adding notes to related documentation help them feel less confused?
- Personally, I find it not tedious to explicitly list the flag values manually (with
1 << index
). This is more explicit and doesn't bloat the code. Yes, it's a bit of a boilerplate, but it's inevitable. To me it looks harmless or almost harmless. You might argue that there is a possibility of a typo, but this is also possible with implicit values. Boilerplate is bad when it spreads across many places in the codebase, but here it is localized within the enum declaration. While a large amount of syntactic sugar in a language creates other problems.
Yes you are right, it is not too bad. 1 << index
is what I've been doing with the current GDScript syntax, and I do feel troublesome from time to time. I personally don't feel like using explicit values in enums or flags, unless indeed necessary. Having to manually assign the values, it feels like instead of for i in range(10)
, you have to write for i in [0,1,2,3,4,5,6,7,8,9]
every time you want to have such a loop. Yes you are right, I'd argue that if your codebase gets a lot of these things, it will just significantly increase the possibility of typo occurrence which we should avoid.
Then the code is loaded down with many unnecessary hard-coded values that could have been handled and inferred more intelligently by the interpreter. What's even worse, for example, when you have to manually assign those flag values, and you then just find out you need to insert or delete some values from the middle, it will be a pain to manually re-order all the values after it. Same thing would happen when you want to merge or split the elements of enums. When there're needs for modification, such as redesigning, refactoring, reordering, or reusing some parts, it becomes a nightmare. Not to mention that for now you need to maintain both the enum and the string arguments list of @export_flags
so it doubles the trouble, you have to always check if they are synced and updated at the same time, and if everything is in the same order.
This painful process happened over and over again during my development, so I am currently using some tool scripts to manage them, but it's still somewhat troublesome. I think it would be much better for the GDScript language itself to provide a builtin solution so other users will have a better native experience without needing to write their own tools.
By the way, how could implicit values also have typo problems? I think I didn't understand this part. Please tell me more! At least if we add the annotation (or keyword), it won't have any impact on any current codebase because they are not using that new annotation. And it will be tested strictly to prevent serious problems, so to me it seems almost harmless to add features like a new auto-increment function…
- We still haven't fully defined the difference between keywords and annotations. There is an "Annotations affect how the script is treated by external tools and usually don't change the behavior." statement in the docs, but we have precedents where annotations significantly affect behavior (
@onready
,@rpc
). However, not every modifier is an annotation; there is a precedent forstatic
. And now we're debating whetherabstract
,private
, etc. Should be keywords or annotations. The same applies to@bitfield
.I understand your concerns and partly agree with them. But, sorry, I won't comment on this in detail to save time. We can discuss naming issues if/when we start implementing the feature. Just to note that the proposed terminology is probably borrowed from the core (
VARIANT_BITFIELD_CAST
macro,is_bitfield
in the docs). Semantic meaning is important, but so is naming consistency, even if it partially distorts the meaning.
That's true. I guess it's some legacy issue and there's an on-going normalization process, and debates as well. Maybe we could discuss it later if we agree on having an implicit flag value function would be a good idea, and go onto the implementation.
Any value in GDScript is a
Variant
(even if statically typed). GDScript is built onVariant
and some other core systems. There are actually not many features in the language that are not supported in the notional "Godot API type system". Operators, method calls, documentation, and more - all this is core stuff on which GDScript depends. It is preferable to change many things in the core rather than trying to add an alternative system to GDScript.We have
GDScriptNativeClass
wrapper that partially emulates the first class types and a number of unresolved issues related toGDScriptNativeClass
. We have unexposedGDScriptFunctionState
forawait
, you can't (or rather shouldn't) obtain it, but we had a few bugs in the VM when it didn't work correctly. In theory, we could add other wrappers, but that looks like a bad idea to me. This would complicate the analyzer and the VM, since they would have to check the compatibility ofint
andBitField[T]
wrapper. It would also complicate cross-language communication. For example, C# might not be aware of the GDScript wrapperBitField[T]
, so we would have to somehow sometimes convert it toint
. Given that the wrapper can be converted toint
, the rationality of the wrapper is lost. Finally, it would have a big performance impact.
Sounds like adding a wrapper at the language level is much more complicated than I thought, and involves a lot of unresolved issues as well. Earlier I was just thinking about the possible impact on performance and memory usage, and how to deal with the difficulties of compatibility with the int
type. Thank you for your patience explaining all those points to me that I hadn't thought of before, and I agree with you. Wrappers in this case sound like a bad idea to me too.
It's possible, here's a proof of concept (
core/variant/variant_call.cpp
):Static void func_int_incremented (Variant *v, const Variant **p_args, int p_argcount, Variant &r_ret, Callable:: CallError &r_error) { int64_t *n = VariantGetInternalPtr<int64_t>:: get_ptr (v); r_ret = *n + 1; } Bind_custom (int, incremented, _VariantCall:: func_int_incremented, true, int);
But the problem I see is that methods of types that are passed by value should be immutable, like in
Vector2
(so instead ofset_flag()
we can only have something likewith_flag()
). We have a number of issues in 3.x where this condition is not met. For example, when calling a mutable method on an array element passed by value, it is actually called on a copy that is in a temporary slot, and the array element doesn't change.
Yes, the immutability of the int
type is one of the things that concerned me before, and the other is that it seems that the bit operation helpers aren't really needed for integers other than bitfields, and exposing those methods to all integers might cause some confusion for people who don't use bitfields (but then again, presumably no one is going to use all of the built-in functions/features, so I guess people are quite used to just leave them alone for the ones they don't find them helpful or sensible).
Seems that without a wrapper we cannot have in-place methods. Let's just still call them set_flag
and so on, because names like with_flag_set
, with_flag_cleared
are longer and not convenient or brief enough. The String
type is also immutable, and have many methods that return a new processed String
instead of modify the original string in-place. But they are still named with the minimal semantic elements such as replace
, reverse
, format
, but are not called with_substring_replaced
and so.
GDScript is not based on Python and is not required to follow its syntax. The asterisk is more like pointers, the ellipsis is more obvious, so I think consistency with C++, JavaScript, PHP, and Godot documentation makes more sense here than with Python. See godotengine/godot-proposals#1034 (comment) and and further discussion.
Yes, the main benefit is that you don't have to duplicate it when declaring multiple variables (like we have to in the core). As I said above, I think
@export_enum
and@export_flags
are for anonymous or one-time enums/flags, plus it's useful if you want to serialize a property as a string (string keys).I'm postponing this due to higher priority issues and because this is quite a complex task in terms of static analysis (plus I have a few refactoring ideas that would simplify the task in the future).
I see now. You are right. *
can be reminiscent of pointers in C++, which can cause confusion.
Is there any chance we can have spread operators for dictionaries as well? So that enums' keys can be unpacked and passed to @export_flags
as arguments. (at least from what I was told by typeof
global function, enums are dictionaries, with string keys and integer values). I tried to get a const Array[String]
by ENUM_NAME.keys()
, but it raises a parse error that it isn't a constant expression - so I guess we can't just use …ENUM_NAME.keys()
as the spread arguments of @export_flags
.
If you want to use GDScript enums, then you should use
@export
instead. Therefore, I consider theBitField[T]
pseudotype necessary for parity with the "Godot API type system".
But did you just mentioned that a wrapper type BitField
can be problematic? Will the BitField[T]
pseudotype be something different and feasible?
Tested versions
System information
Godot v4.2.1.stable - macOS 14.3.1 - Vulkan (Forward+) - integrated Apple M2 Max - Apple M2 Max (12 Threads)
Issue description
I have a question about the
@export_flags
bit fields (which are under the hood integers), and not sure if it is a bug or not. The documentation of@export_flags
states that:And also if you try in the engine to give more than 32 arguments to
@export_flags
it will raise the parse error below:So both the documentation and the error message sound like the bit fields can only hold 32 bits of information. However, I also read that in documentation it also says the
int
type has a size of 64 bits or 8B. I also performed some tests on aint
variable that has@export_flags
annotation, it shows that an@export_flags
field can indeed store 64 bits information.I am not sure what makes
@export_flags
32 bits. Is it a bug, a legacy issue, or an intended behavior (for example some compatibility strategy in regard to 32-bit architectures/OS)?Thank you for your clarification!
Steps to reproduce
N/A
Minimal reproduction project (MRP)
N/A