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 and small optimization #2

Closed

Conversation

Ward727a
Copy link

So as I said here: Redot-Experimental#11 (comment) there was a bug when we get 2 functions on the same line in trait file and script file. This is now fixed!

Also I did a small optimization on the get_function_names_recursively function inside gdscript_editor.cpp this should allow us to gain ~10% (from 9.8µs to 8.8µs).

In fact: The fix and the optimization are linked.

Before when Redot was getting the function name inside the script, it was checking for the function name inside the script file (.gd) and added them to a HashMap<int, String> (int = starting line of the function, string = function name), then it was checking for the function name inside the traits files (.gdt), but then if 1 function was on the same line than the script file, the function inside the traits files would "override" it.

Now they are "pre-concat" then added to a vector.

Some explanation:

First, a simple example:

# MyTrait.gdt
trait_name my_trait extends Node

func MyTraitFunction():
    pass
    
# MyScript.gd
extends Node
uses my_trait
func _process():
    print("hello world!")

func _ready():
    pass

Here we have 2 functions on the same line (3): MyTraitFunction and _process, Redot would first get the function from the script so MyScript.gd here, giving this result:

Function[3] = "_process"
Function[6] = "_ready"

Then Redot would get the function from the trait, here MyTrait.gdt, but as the function MyTraitFunction is on the same line, here is the result:

Function[3] = "MyTraitFunction"
Function[6] = "_ready"

We can see that we losed "_process".

In this fix, as we use vector, we directly format the string as awaited for next function, with the new version, we would get something like that:

Function = ["_process:3", "_ready:6", "MyTraitFunction:3"]

decryptedchaos and others added 9 commits February 12, 2025 13:00

Unverified

The signing certificate or its chain could not be verified.

Verified

This commit was created on github.com and signed with GitHub’s verified signature.
#Conflicts:
#	modules/gdscript/gdscript_cache.cpp

Verified

This commit was created on github.com and signed with GitHub’s verified signature.
- Now when we create a trait, the first line is "trait_name trait extends parent".
- Methods list now doesn't show non-existant function inherited from the trait.
- Fixed the ability to create 2 GDT with the same name.
- Fixed a gutter icon bug for non-existant function that where inside the trait but not inside the script.
…ript) was one the same line: The gutter icon for virtual function wasn't shown.

- Did a little optimization on the "get_function_names_recursively" function (using Vector and not HashMap) this allow us to gain ~10% in time when running (HashMap average ~= 9.8µs / Vector average ~= 8.8µs).
@decryptedchaos
Copy link
Owner

So, i'm probably gonna have to just pull the diff here and do fix this PR on my side because i had to create a new branch for the trait feature called feature/traits-fix

No big deal i can fix it, but it does mess up the credit on the log so i'll just reference this PR

@Ward727a
Copy link
Author

Okk, no problem at all, don't worry :)

@decryptedchaos decryptedchaos changed the base branch from feature/traits to feature/traits-fix February 19, 2025 22:19
@decryptedchaos decryptedchaos changed the base branch from feature/traits-fix to feature/traits February 19, 2025 22:19
@decryptedchaos decryptedchaos changed the base branch from feature/theme to feature/traits-fix February 20, 2025 00:47
@decryptedchaos
Copy link
Owner

Closing this, We need to re-open a new PR with the correct branch this branch was very broken and i had to create a new branch to fix it

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