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

Refer units by type instead of gd file #105

Open
lmorao opened this issue Feb 25, 2024 · 1 comment
Open

Refer units by type instead of gd file #105

lmorao opened this issue Feb 25, 2024 · 1 comment
Labels
refactoring Change in the code without functional impact

Comments

@lmorao
Copy link

lmorao commented Feb 25, 2024

Hello,

there's a lot of paths used in the files, like for example in
res://source/match/units/actions/CollectingResourcesSequentially.gd

const Worker = preload("res://source/match/units/Worker.gd")
static func is_applicable(source_unit, target_unit):
	return (
		(source_unit is Worker and target_unit is ResourceUnit)
		or (source_unit is Worker and target_unit is CommandCenter and target_unit.is_constructed())
	)

As an alternative, I see that the base unit class has a type setting.
My question is, won't it be easier in general to use unit.type == "Worker" instead of doing unit is Worker?

This would simplify the example above to not require the const

static func is_applicable(source_unit, target_unit):
	return (
		(source_unit.type == "Worker" and target_unit is ResourceUnit)
		or (source_unit.type == "Worker" and target_unit is CommandCenter and target_unit.is_constructed())
	)

The type function doesn't exist on structures, but it would be easy to copy paste it there so that this can be extended to structures as well.

Is there a drawback to doing this?
Right now the current drawback is that we can't move files around, and also if I want a second type of worker, I need to go to all the files that use the Worker and add another worker reference path, and change the logic to include the other worker type for all the checks.

Conversely with this change, I could make a new worker, "hardcode" the type to "Worker" and it would just work.

Regards,
Luís

@Scony Scony added proposal Feature to be considered refactoring Change in the code without functional impact and removed proposal Feature to be considered labels Feb 26, 2024
@Scony
Copy link
Contributor

Scony commented Feb 26, 2024

Hi, the unit.type was added fairly recently. Anyway, you're probably right that it could be used instead of precise type checking. The drawback is ofc. that it's prone to name collisions if you have 2 different Worker scenes in different parts of the filesystem tree.
As for reusability, IMO the type should not be used. The type should be used to uniquely identify units so that e.g. you can distinguish between 2 different types of workers. If you'd like to introduce some functionality common to all worker types you should rather add each worker to group called e.g. workers and then check if unit belongs to that group.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Change in the code without functional impact
Projects
None yet
Development

No branches or pull requests

2 participants