Caching project settings variables

[TheYellowArchitect]

🐛 Description

ProjectSettings.get_setting boils down to a Node.get call under the hood, so I'd say it's fine as it is. But if you want to be proactive about perf, feel free to introduce a variable with an underscore prefix that's initialized in _ready and use that.

According to the above, all invocations of ProjectSettings.get() call Node.get() which I assume is a property getter (via string!)
ProjectSettings.get_setting is invoked at least 5 times per tick in RollbackSynchronizer so as the quote says, all of these project settings which are read-only should be cached.

Example code below for reference of current project settings implementation

## How many ticks to store as history.
##
## The larger the history limit, the further we can roll back into the past, 
## thus the more latency we can manage.
##
## [i]read-only[/i], you can change this in the project settings
var history_limit: int:
	get:
		return ProjectSettings.get_setting("netfox/rollback/history_limit", 64)
	set(v):
		push_error("Trying to set read-only variable history_limit")

and from what I understand should be done (correct me if I'm wrong)

var _history_limit: int
## How many ticks to store as history.
##
## The larger the history limit, the further we can roll back into the past, 
## thus the more latency we can manage.
##
## [i]read-only[/i], you can change this in the project settings
var history_limit: int:
	get:
		return _history_limit
	set(v):
		push_error("Trying to set read-only variable history_limit")

...

func _ready():
      _history_limit = 64

So probably a function at _ready() is required to be added, which sets all starting values for the project setting variables.

Btw, I suggest first doing a search in godotengine if it plans to cache the project settings variables, so we don't implement it here and have it be needless by godot in the future.

[elementbound]

all invocations of ProjectSettings.get() call Node.get() which I assume is a property getter (via string!)

Don't assume, do research or ask questions. For the former, peruse the relevant documentation and Godot source code, for the latter, use the Discord or the discussions tab here on Github. Also, if you don't go through the effort of validating your assumptions about Godot, why should any contributor - including myself - go through the trouble of validating it for you?

Btw, I suggest first doing a search in godotengine if it plans to cache the project settings variables, so we don't implement it here and have it be needless by godot in the future.

This is misdirected in a few ways. First off, you seem to be assuming that the current implementation is not fast enough. Second, I have to reiterate, how do you expect any contributor to put in the effort to research anything, if you can't do the same about the issue you're trying to raise. Third, if you're not sure about something needing to be done, then there's no point in opening an issue. I can't fix something that is maybe broken or maybe not. Especially in this case, where nothing is broken ( hence the removed bug label ).

---

I'm closing this issue, as - at the time of writing - there's nothing to fix. Feel free to raise this issue again, if you have factual evidence that the ProjectSettings.get_setting calls measurably slow down netfox. Otherwise we end up spending time on it without reaping any actual benefits.

[TheYellowArchitect]

if you have factual evidence that the ProjectSettings.get_setting calls measurably slow down netfox. Otherwise we end up spending time on it without reaping any actual benefits.

Measurably as in benchmark? If yes, do re-open and I will try a benchmark when I find time. This is not a non-issue. This is a hotpath, and not caching variables which are invoked every tick, at so many nodes surely costs performance, GDScript isn't free.

First off, you seem to be assuming that the current implementation is not fast enough.

GDScript is very slow, and this is a hotpath. If I run 200 nodes/rollback synchronizers, this will certainly drop FPS for no good reason. It's not like the solution to this issue would bloat the code, its just a few variable declarations.

Second, I have to reiterate, how do you expect any contributor to put in the effort to research anything, if you can't do the same about the issue you're trying to raise.

Putting effort to research something, might as well make a PR for this. I am only reporting the problem. If it is deemed to be a false flag, you have the power to shut it down. I am doing my duty pointing out what I see is a flaw. Using a RollbackSynchronizer in an RTS where a hotpath calls Node.get() to a property via string, for every node, every tick should be costly. If you think it isn't, as you did, you close this issue, no harm done.

Third, if you're not sure about something needing to be done, then there's no point in opening an issue

I am sure something needs to be done, hence the code above which is essentially the PR template. The only uncertainty is whether godotengine itself plans to implement ProjectSettings caching and that would require me search some minutes through all godot proposals and godot PRs, to ensure it isn't already a thing.