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

@export_flags integer size concerns #89916

Open
lethefrost opened this issue Mar 26, 2024 · 9 comments
Open

@export_flags integer size concerns #89916

lethefrost opened this issue Mar 26, 2024 · 9 comments

Comments

@lethefrost
Copy link

Tested versions

  • Reproducible in 4.2.1

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:

Note: A flag value must be at least 1 and at most 2 ** 32 - 1.

And also if you try in the engine to give more than 32 arguments to @export_flags it will raise the parse error below:

Parse Error: Invalid argument 33 of annotation "@export_flags": Starting from argument 33, the flag value must be specified explicitly.

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 a int 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

@AThousandShips
Copy link
Member

AThousandShips commented Mar 26, 2024

I'd say this makes more sense as a proposal, as there's no bug here but a limitation that could be discussed

@dalexeev
Copy link
Member

void EditorPropertyFlags::_flag_toggled(int p_index) {
uint32_t value = get_edited_property_value();

@lethefrost
Copy link
Author

void EditorPropertyFlags::_flag_toggled(int p_index) {
uint32_t value = get_edited_property_value();

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.

@dalexeev
Copy link
Member

I'm not sure about the reasons, but my guess is:

  1. Even though 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.
  2. 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.
  3. 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.

@lethefrost
Copy link
Author

lethefrost commented Mar 27, 2024

I'm not sure about the reasons, but my guess is:

  1. Even though 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.
  2. 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.
  3. 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 (#74418 , #71234 (comment), godotengine/godot-proposals#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:

  • some global helper functions for bitfield operations (I am not sure if the int type can have methods like Strings do, if it does, then adding those methods to int could be preferred).
  • a @bitfield annotation for enums as Improve usability of bitflag fields in GDScript godot-proposals#923 (comment) 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.
  • 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

Does it sound okay to you?

Thank you so much again for your time and patience!

@dalexeev
Copy link
Member

dalexeev commented Mar 27, 2024

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.

@lethefrost
Copy link
Author

Hi Danil! Sorry for the late response, and thank you for providing those helpful information and sharing your thoughts!

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 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 enums.

# 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:

  • bitfield: They are usually integer variables stored on the stack, or stored as member properties of an object, for the purpose of later using and checking the stored values.
    • It emphasizes its identity as a field, indicating its usage in storing and representing data within a defined memory space, often compactly encapsulating multiple boolean conditions or settings.
  • bit flags: Typically refers to integer constants with only one bit set to 1, which is a single set bit, used to represent the set state of the nth position.
    • Flags emphasize representing the status of specific positions, highlighting their role in signaling the presence or absence of various conditions or features by setting individual bits within an integer.
  • bit mask: Resulted from doing bitwise AND on specific position's bit flags, may have one or more bits set to 1. It is used on one side of a bit operation, for instance, to perform bit operations such as check, toggle, set, clear on a group of specific positions of bits in a stored bitfield value.
    • It emphasizes its role as a mask in computational operations with another bitfield, underscoring its use in selectively applying bitwise operations to manipulate, query, or combine bits from one or more bitfields.

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 enums, 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 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.

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 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).

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:

  1. 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.
  2. @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:

  1. 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)

  2. 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:

  1. Add new @bitflags annotation to enum types, which will:
    1. Turn their identification in documentation as flags;
    2. Turn their default values to be powers of 2
  2. Either add a new type BitField (which is a wrapper of int) or not,
    1. If BitField added:
      1. Allow @export var spell_elements: BitField[ATTACK_TYPE] to export checkbox group to inspector;
      2. Add bit operation helper methods to BitField
    2. If BitField not added:
      1. Allow for @export_flags(ATTACK_TYPE) var spell_elements: int overload or replace the original variadic string parameters;
      2. Add bit operation helper methods to int

I hope my statements make some sense. Let me know!

Thank you.

@dalexeev
Copy link
Member

dalexeev commented Mar 29, 2024

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 enums.

I'm not strictly against this, but I have some concerns:

  1. Enumerations have an auto-continue feature. For example, 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
  2. 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).
  3. 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.
  4. 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 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 enums, 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 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?

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 the int type can have methods as other types or classes do, because I saw what are currently defined on int 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 godotengine/godot-proposals#1034 (comment) and and further discussion.

  1. 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).

@lethefrost
Copy link
Author

  1. Enumerations have an auto-continue feature. For example, 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

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.

  1. 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?

  1. 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…

  1. 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 for static. And now we're debating whether abstract, 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 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.

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 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.

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 the BitField[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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants