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

Allow for signal events to optionally pass dictionaries instead of strings #1829

Merged

Conversation

Pheubel
Copy link
Contributor

@Pheubel Pheubel commented Oct 27, 2023

This would resolve #1828

I made this mostly to demonstrate how i envision it to work and think it works. I tried playing around with it and am fairly satisfied.

What I added/changed:

  • An input scene that acts similar to an array, but instead there are two fields per entry, a key and a value. To keep it simple enough, values are exclusively strings, rather than allowing any data type.
  • Added resource type KEY_VALUE_PAIRS, i put it under the OTHER category in the enum. This type is meant for dictionaries, which are key value pairs. in to_text() they are handled under their own clause, as putting them in var_to_str() would cause newlines to be added and breaks the syntax highlighting for the text editor. Instead they get serialized densely; example: arg="{"a":"1","b":"2"}"
  • Added a toggle to switch between the signal using a string as an argument and a dictionary as an argument. It defaults to false and should be compatible with existing signals due to how the syntax works. Since you need to opt-into dictionaries as argument rather than a string, existing code does not break. But in order to take advantage of the feature, they do need to change their code to handle a dictionary argument. With a simple type filter it should work like a charm.
# a simple example of how a type filer could look
func on_timeline_signal(arg: Variant) -> void:
	match typeof(arg):
		TYPE_STRING:
			handle_string_argument(arg as String)
		TYPE_DICTIONARY:
			handle_dictionary_argument(arg as Dictionary)

What can still be added:

  • For the key value pair inputs, a user could define the type per value, rather than locking them to just strings. However for this to be reliable, it would need a custom serialization step, rather than using Godot's built in JSON stringifier. As it follows the JSON specs, it treats all read values as floats since JSON doesn't distinguish between the two, it only has a number type for numbers. This increases the complexity of implementing this feature and could possibly be added in the future.
  • Better wording for the visual editor of the signal node. I found it challenging to come up with a good short description that doesn't confuse people. I'm more a technical minded guy and am generally not good with describing things like this after a code jam.

I hope you give this feature a try and see if this is a good fit

the key value is not the smartest cookie, it only goes one level and values are exclusively strings, but this should be fine for now.
without it, JSON would output an error while the result would not be as relevant to the developer IMO
@Jowan-Spooner
Copy link
Collaborator

I like the idea and the implementation isn't bad either, but UI/UX need improvements. I will try to improve it and then merge it.

@Jowan-Spooner
Copy link
Collaborator

Okay I made a pass on UI/UX/naming:

Default look:

grafik

Default expanded:

grafik

After switching to Dictionary:

grafik

With some values:

grafik

In text:

grafik
grafik

@Jowan-Spooner
Copy link
Collaborator

The dictionary UI looks really rought, but it's based of the array UI which looks similarly bad, so I'll leave that for a future PR.
I'd also consider both in array and dictionary uses to do a parse_variables pass to allow easy passing of dialogic variables in those.

@Jowan-Spooner Jowan-Spooner merged commit 90d94d8 into dialogic-godot:main Oct 28, 2023
@Jowan-Spooner
Copy link
Collaborator

Thanks for this feature!

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

Successfully merging this pull request may close these issues.

[Suggestion] Allow signal event pass a dictionary rather than just a single string
2 participants