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

fix: move ESCCondition parsing logic out of the constructor #713

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

bolinfest
Copy link
Contributor

fix: move ESCCondition parsing logic out of the constructor

This makes it possible to create an ESCCondition programmatically.
Unfortunately, this does not fix the issue with ESCCondition
where comparison_regex.compile(REGEX) is called every time an
ESCCondition is instantiated.

I have read all sorts of forums, and no one seems to address this
issue head-on. The question of, "Why doesn't GDScript support static
variables" is not addressed clearly in the docs, IMHO.

The only "workaround" I can find is this article on Singletons/Autoload:

https://docs.godotengine.org/en/latest/tutorials/scripting/singletons_autoload.html

Though this seems incredibly heavyweight for caching a RegEx.

@bolinfest
Copy link
Contributor Author

Note that the PR does something very wrong, as-is, which is it passes {} as the first argument to logger.

I see that sometime in the past 18 months, the Logger API was changed to require an owner object of some sort. When updating my own project, this seemed to make it impossible to use from a static function. I ended up deleting the logger statements because I couldn't find a workaround.

This makes it possible to create an `ESCCondition` programmatically.
Unfortunately, this does not fix the issue with `ESCCondition`
where `comparison_regex.compile(REGEX)` is called *every time* an
`ESCCondition` is instantiated.

I have read all sorts of forums, and no one seems to address this
issue head-on. The question of, "Why doesn't GDScript support static
variables" is not addressed clearly in the docs, IMHO.

The only "workaround" I can find is this article on Singletons/Autoload:

https://docs.godotengine.org/en/latest/tutorials/scripting/singletons_autoload.html

Though this seems incredibly heavyweight for caching a `RegEx`.
StraToN
StraToN previously approved these changes Oct 28, 2023
@StraToN
Copy link
Collaborator

StraToN commented Oct 28, 2023

This change looks interesting, thank you for it.
It is right that GDScript is a bit limited when we're starting to require some higher level language features. Hopefully this will be leveraged in the future - either when we move to Godot 4.x, or use C#, which is not in our plans at the moment.

@bolinfest
Copy link
Contributor Author

Though before anyone merges this, nb:

Note that the PR does something very wrong, as-is, which is it passes {} as the first argument to logger.

How should I fix that?

@BHSDuncan
Copy link
Collaborator

@StraToN do we want to merge this in despite us moving to the new interpreter?

@StraToN
Copy link
Collaborator

StraToN commented Feb 15, 2024

Though before anyone merges this, nb:

Note that the PR does something very wrong, as-is, which is it passes {} as the first argument to logger.

How should I fix that?

Logging methods require the calling class so its name is put in the log message. You may simply pass self.

@StraToN
Copy link
Collaborator

StraToN commented Mar 28, 2024

@bolinfest Any news on this? I think it only needs the logger parameter to be set. When this is done I am fine merging this @BHSDuncan no matter if we're to moving to the new interpreter.

@bolinfest
Copy link
Contributor Author

Logging methods require the calling class so its name is put in the log message. You may simply pass self.

Is self valid in a static function in Godot? ChatGPT doesn't think so:

https://chat.openai.com/share/9d22a90e-c1d4-4ca6-8c34-21f6b6d25370

@StraToN
Copy link
Collaborator

StraToN commented Mar 29, 2024

Logging methods require the calling class so its name is put in the log message. You may simply pass self.

Is self valid in a static function in Godot? ChatGPT doesn't think so:

chat.openai.com/share/9d22a90e-c1d4-4ca6-8c34-21f6b6d25370

Ah, ok I missed your point.

Here is what I can propose then: add a new error_static() method in ESCLogger that you should be able to call in a static way:

	# Error log
	func error(owner: Object, msg: String):
		var context = owner.get_script().resource_path.get_file()
		error_static(context, msg)

	func error_static(context: String, msg: String):
		printerr(formatted_message(context, msg, "E"))
		push_error(formatted_message(context, msg, "E"))
		if ESCProjectSettingsManager.get_setting(
			ESCProjectSettingsManager.TERMINATE_ON_ERRORS
		):
			assert(false)
			escoria.get_tree().quit()

You can then call it like so:

escoria.logger.error_static(
							"ESCConditionParser",
							"Invalid comparison type detected: %s" %
									comparison_string +
							"Comparison type %s unknown" %
									ESCUtils.get_re_group(
										result,
										"comparison"
									)
						)

@StraToN StraToN self-requested a review March 31, 2024 20:40
@StraToN StraToN dismissed their stale review March 31, 2024 20:41

Wait for #718 to be merged and apply logger "static" calls

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.

3 participants