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

Match don't match #42224

Open
Ducote opened this issue Sep 21, 2020 · 22 comments · May be fixed by #98275
Open

Match don't match #42224

Ducote opened this issue Sep 21, 2020 · 22 comments · May be fixed by #98275

Comments

@Ducote
Copy link

Ducote commented Sep 21, 2020

Found nothing like my issue with : is:issue "match" in:title label:bug state:open searching's filter.

Godot version:
Godot Engine v3.2.3.stable.official

OS/device including version:
Ubuntu 20.04
GPU : Nvidia GeForce GTX 970
GPU driver : nvidia driver 450 (libre)
Proc : AMD® Ryzen 9 3900x

Issue description:
Match is supposed to match with the first option... but don't.
Capture d’écran de 2020-09-21 12-37-19

Steps to reproduce:
Don't know too much.
Made a variable match with a enum's const.

Minimal reproduction project:
issue_minimal_project.zip

@Calinou
Copy link
Member

Calinou commented Sep 21, 2020

I can reproduce this on Godot 3.2.3.

That said, the first time I launched the project, I had a different output when clicking the Artistes/Albums button (it toggled between both states).

It works if you add an int type hint to your var declaration:

var searching_mode: int = searching_by.ARTIST

The variable's type can't be inferred for some reason…

@Calinou Calinou added the bug label Sep 21, 2020
@Ducote
Copy link
Author

Ducote commented Sep 21, 2020

I have the same bug at different location on my script, and it's the same. Thanks for your tips, it works !

What's crazy is that there is a part of my code that worked perfectly fine... Then just broke...

@ShalokShalom
Copy link

ShalokShalom commented Sep 21, 2020

@karroffel implemented this, so he might be able to help. 🙂

@Calinou
Copy link
Member

Calinou commented Sep 21, 2020

@ShalokShalom karroffel no longer contributes to Godot core, so I'm afraid he can't help much here.

@strank
Copy link
Contributor

strank commented Sep 21, 2020

I just tested this on the current master and could not reproduce it. So at least it seems to have been fixed during the GDScript rewrite for 4.0.

@Ducote
Copy link
Author

Ducote commented Sep 21, 2020

That's good to hear ! Just have to wait for 4.0..... JUST ! ^^

@ThakeeNathees
Copy link
Contributor

ThakeeNathees commented Sep 22, 2020

JSON parse 0 as 0.0 float and you can't match float 0.0 with int 0 (not sure if it's a bug CC @vnen ) but the bug here is if a float value is a whole number it just prints the integer and ignore the decimal point which is harder for someone to debug.

var value = JSON.parse('0').result ## <-- a float value not integer

func _ready():
	print(value) ## <-- prints 0 not 0.0
	match value:
		0:
			print('0')
		1:
			print('1')
		0.0:
			print('0.0') ## <-- matches here
		_:
			print('default')

@Calinou
Copy link
Member

Calinou commented Sep 22, 2020

@ThakeeNathees GDScript is a strongly-typed language, so 0 not being equal to 0.0 is a design decision, not a bug.

@Ducote
Copy link
Author

Ducote commented Sep 22, 2020

Then, why do the if condition work, but match doesn't ?

@vnen
Copy link
Member

vnen commented Sep 25, 2020

== works on int and float arguments mostly for convenience. But match is type-strict all the time, since some comparisons would be invalid (if you try to use == with incompatible types you get an error).

So this isn't really a bug, you should cast for the type you expect it to be.

Now I wonder if I broke something on 4.0 or if the test was made with something different, because it should still only match if the type is the same.

@strank
Copy link
Contributor

strank commented Sep 25, 2020

@vnen my testing mentioned above didn't go through the json parser, my bad. it seems that it's still type-strict in 4.0

@strank
Copy link
Contributor

strank commented Sep 25, 2020

BTW, I think that matching a pattern with a literal, variable or constant should use the same mechanism and provide exactly the same convenience that the equality test == in that language provides. The reason: to avoid exactly the misunderstanding that led to this issue report.
It is also in line with other languages that support some duck-typing, AFAIK. E.g. case in Ruby (I think) and the pattern matching statement proposed for python https://www.python.org/dev/peps/pep-0622/#patterns

But I guess, this change would have to go into a new proposal.

@Poobslag
Copy link

I've encountered this same problem.

Interestingly, the match statement works OK when launched from the editor, and works OK when exported to Windows with debug mode enabled. The match statement only fails when exported to Windows with debug mode disabled.

Adding an explicit type of 'int' fixes the match statement.

@mojoyup
Copy link

mojoyup commented Sep 20, 2022

Im getting similar results with 4.0 beta 1 that just came out. I wanted to wait to convert until functions were not changed anymore :P. So, match is not firing for me when I try to retrieve a poolbtyearray from firestore. Here is my code. I can print the response_code, which is what match should recognize. Thoughts?
Capture

@dongohu
Copy link

dongohu commented Sep 26, 2023

I have been working over 9 hours straight due to this issue thinking the problem was elsewhere.
I'm working with Godot 4.1.2rc1

The following codes is used in my project to controls the value (and if active) of a TextureRect node.
A typical Pixelation effect shader is applied to it with an Shader Parameter called "Pixelation".

Here's the code using a match statement:
(Yes, I'm using semicolons in my coding. Can't shake off that habit.)

extends CanvasLayer

@export var NP_PixelatedMaterial: NodePath;
@export var TextRend_PixelsRenderer: TextureRect;

var CurPixelMaterial: Material;

func SetPixelationEffectValue(val: int):
	print(val);
	CurPixelMaterial = TextRend_PixelsRenderer.material;
	match val:
		_:
			#Normal Pixels (448px)
			get_node(NP_PixelatedMaterial).show();
			CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 448);
		0:
			#Large Pixels (256px)
			get_node(NP_PixelatedMaterial).show();
			CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 256);
		2:
			#Small Pixels (768px)
			get_node(NP_PixelatedMaterial).show();
			CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 768);
		3:
			#Disabled
			get_node(NP_PixelatedMaterial).hide();
			CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 1024);

Even if I verify (print) that the val integer has the proper value, the match always applies the _ (default) value.

If, instead, I use this:

extends CanvasLayer

@export var NP_PixelatedMaterial: NodePath;
@export var TextRend_PixelsRenderer: TextureRect;

var CurPixelMaterial: Material;

func SetPixelationEffectValue(val: int):
	print(val);
	CurPixelMaterial = TextRend_PixelsRenderer.material;
	if val == 0:
		#Large Pixels (256px)
		get_node(NP_PixelatedMaterial).show();
		CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 256);
	if val == 1:
		#Normal Pixels (448px)
		get_node(NP_PixelatedMaterial).show();
		CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 448);
	if val == 2:
		#Small Pixels (768px)
		get_node(NP_PixelatedMaterial).show();
		CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 768);
	if val == 3:
		#Disabled
		get_node(NP_PixelatedMaterial).hide();
		CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 1024);

It works 100%.
This has happened quite a few times in my current project where match always returns the _ (default) value even if the matched value is not it.

A side note to the issue:
On a different game engine, this was a common problem ~7 years ago and was due to a compiling & caching issue which made it so that the switch statements (a.k.a. match statement) were cut short to a fraction of their regular content. Like having 48 cases cut down to 27 cases. It didn't returned any compiling error.
Back then, the only fix was to use if() (and its relative options) in replacement until the compiler was updated.
I can't say if this is the case for Godot, but that's a start.

@ShalokShalom
Copy link

ShalokShalom commented Sep 26, 2023

@dongohu Considering the comment from Poobslag, do you also experience this bug only, when you export to Windows with no debug mode enabled?

@dongohu
Copy link

dongohu commented Sep 26, 2023

@dongohu Considering the comment from Poobslag, do you also experience this bug only, when you export to Windows with no debug mode enabled?

No, I experience this via:
• When running the game through the Run Project (F5) from the editor.
• When exporting the game to Windows Desktop (both in Debug and without Debug)

There's no error or issues being pointed out, but every instance of match I uses in GDscript seems to return the default value.

= EDIT =

Ha! I just found one thing that works with match, but this reflects that the problem is the engine's interpretation of the match cases.

If you place the default _: case as the last case, it works!

@export var NP_PixelatedMaterial: NodePath;
@export var TextRend_PixelsRenderer: TextureRect;

var CurPixelMaterial: Material;

func SetPixelationEffectValue(val: int):
	print(val);
	CurPixelMaterial = TextRend_PixelsRenderer.material;
	match val:
		0:
			#Large Pixels (256px)
			get_node(NP_PixelatedMaterial).show();
			CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 256);
		2:
			#Small Pixels (768px)
			get_node(NP_PixelatedMaterial).show();
			CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 768);
		3:
			#Disabled
			get_node(NP_PixelatedMaterial).hide();
			CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 1024);
		_:
			#Normal Pixels (448px)
			get_node(NP_PixelatedMaterial).show();
			CurPixelMaterial.set_shader_parameter(References.Ref_ShaderParam_Pixelation, 448);

This is the revision of my previous code and this works.
If I'm not wrong, could it be that the engine interpret the match as if's and replace _: for else without reordering?
This would explain why, whenever _: is not last, the rest is skipped right away.

I'm kinda happy that I found this work-around now while I'm still working on the small stuff that is relatively easy to review because I'm on the verge of working on a dynamic world generation system that I build in another engine that is using match statement as its basics for determining the biomes and its content from simple values.

@vnen
Copy link
Member

vnen commented Oct 19, 2023

If I'm not wrong, could it be that the engine interpret the match as if's and replace _: for else without reordering?
This would explain why, whenever _: is not last, the rest is skipped right away.

There's no reordering done and it is not expected to be. If the wildcard is the first pattern it will always match anything. You do get a warning if you add patterns after a wildcard or unconditional variable bind.

@dongohu
Copy link

dongohu commented Oct 19, 2023

If I'm not wrong, could it be that the engine interpret the match as if's and replace _: for else without reordering?
This would explain why, whenever _: is not last, the rest is skipped right away.

There's no reordering done and it is not expected to be. If the wildcard is the first pattern it will always match anything. You do get a warning if you add patterns after a wildcard or unconditional variable bind.

That's actually something relatively unique to Godot then and there is no warning currently for having anything after the wild card in Godot 4 neither during the coding nor actual test/play phases. (Otherwise, I would have noticed it and would have corrected the code right away and wouldn't have taken as much time finding the issue.)

To give examples, Unity, Unreal, CryEngine and even RPG Maker apply a reordering to the Switch function, during compilation for Android and iOS build, which replace switch statements by if/ifelse/else functions which is why I though it could have something to do with it. On other builds, they are managing the switch function by only returning the wild card if none of the other expressions are true.

To quote Microsoft's own documentation on switch function:

The Switch function argument list consists of pairs of expressions and values. The expressions are evaluated from left to right, and the value associated with the first expression to evaluate to True is returned. (...) Switch evaluates all of the expressions, even though it returns only one of them.

Godot is the only case I have found that uses a true value for its wild card for its switch function instead of using a null value as, otherwise, it would detect that a later expression is true and would only return the wild card if none are true.

@vnen
Copy link
Member

vnen commented Oct 19, 2023

@dongohu match is not the same as switch. They serve similar purposes but they work differently. In Rust, for instance, patterns are evaluated in order and it warns for unreachable patterns, just like GDScript does.

And there is a warning for that. I do think warnings are a bit hidden in the editor, but the warning exists:
Screenshot 2023-10-19 at 11 55 44

@ShalokShalom
Copy link

@dongohu switch in C# is a flange-mounted pattern matching, match in GDScript is a glorified if-then-else statement.

The difference explained.

@dalexeev
Copy link
Member

dalexeev commented Aug 8, 2024

As stated above and described in the GDScript documentation:

1. match treats int and float as unequal, unlike the == operator. You should be careful when getting numeric values ​​from JSON and other external sources.

Note

We could add a warning when the types of matching and pattern values ​​are known, unequal, and implicitly convertible to each other. However, this may have false positives.

Docs

2. Wildcard (_) and binding (var x) patterns are universal, the order of match branches is important, so no patterns should appear after a first-level universal pattern. There is already the warning UNREACHABLE_PATTERN.

Docs



So the only actionable suggestion I see here is to add a warning when trying to compare int and float. Let me know if I missed anything.

@dalexeev dalexeev added discussion and removed bug labels Aug 8, 2024
@dalexeev dalexeev moved this from For team assessment to Up for grabs in GDScript Issue Triage Aug 8, 2024
@WizardOhio24 WizardOhio24 linked a pull request Oct 17, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Up for grabs
Development

Successfully merging a pull request may close this issue.

10 participants