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

Add a method for acquiring a single key from Dictionary #11447

Open
tehKaiN opened this issue Dec 30, 2024 · 2 comments
Open

Add a method for acquiring a single key from Dictionary #11447

tehKaiN opened this issue Dec 30, 2024 · 2 comments

Comments

@tehKaiN
Copy link

tehKaiN commented Dec 30, 2024

Describe the project you are working on

A hierarchical destructible system.

Describe the problem or limitation you are having in your project

In my code, I need to flood-fill through the connected nodes on my custom graph data structure and assign some values, do some processing, or just grab an interconnected subset of nodes after destructible gets split in half. Since HashSet or similar collections aren't available in GDScript, I'm using a Dictionary[T, bool] to store a set of objects of type T.

I've ran into limitation that I can't just grab any element from the set - I need to call keys() first to grab all the keys and then just get one element and discard the rest.

static func extract_connected_subset(remaining: Dictionary[Block, bool]) -> Dictionary[Block, bool]:
	var contained: Dictionary[Block, bool] = {}
	var front: Dictionary[Block, bool] = {}
	var first := remaining.keys()[0] as Block  # "get next element from set"
	front[first] = true
	while !front.is_empty():
		var block := front.keys()[0] as Block  "get next element from set"
		front.erase(block)
		contained[block] = true
		remaining.erase(block)
		for neighbor: Block in block.neighbors:
			if !contained.has(neighbor):
				front[neighbor] = true
	return contained

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The var element := some_dict.keys()[0] as T could be replaced with var element := some_dict.get_any_key() as T.

This is not only cleaner, but also has significant performance implications because the keys() method iterates through all the dictionary keys and puts them in a freshly allocated container. In my use case most of that work is wastefully discarded afterwards.

I thought about exposing get_key_at_index() method to GDScript, but that doesn't seem to be that useful, and might be misleading to scripters because of raising assumptions on how the elements are ordered in the dict.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

	Variant get_any_key() const {
		if(is_empty()) {
			return Variant();
		}
		return _p->variant_map.begin()->key;
	}

alternatively:

	Variant get_any_key() const {
		return get_key_at_index(0);
	}

Naturally, I'm willing to submit PR with needed code/tests/docs.

If this enhancement will not be used often, can it be worked around with a few lines of script?

It can be worked around as I did in my example code, but I don't see any performant way to get a single key from the dictionary.

Is there a reason why this should be core and not an add-on in the asset library?

I'm not sure - can addons/modules add GDScript methods to core types? Besides, it solves a problem in what appears to be a relatively common scenario.

@dalexeev dalexeev changed the title GDScript: Add a method for acquiring a single key from Dictionary Add a method for acquiring a single key from Dictionary Dec 30, 2024
@kleonc
Copy link
Member

kleonc commented Dec 30, 2024

As a workaround for better performance you could use the fact that iterating over a dict doesn't generate the keys array:

func get_first_key(dict: Dictionary):
	for key in dict:
		return key

But when replacing var block := front.keys()[0] as Block with var block: Block = get_first_key(front) etc. note that there's an additional cost of a function call, which might overweight the gain of faster key obtaining (in case of small key count). It should be relatively negligible but for being sure do the benchmarks.
Alternatively instead of calling a function you could inline it:

var block: Block
for b: Block in front:
	block = b
	break

# Same as an ugly one-liner:
#var block: Block; for b: Block in front: block = b; break

@tehKaiN
Copy link
Author

tehKaiN commented Dec 31, 2024

I think that calling dedicated method won't be slower than calling keys() as in current state. Unless calling it is handled differently?

As for the loop workaround, it's quite a novel approach, but I can't see myself riddling my code with such constructs - the legibility of GDScript goes down the drain with it.

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