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.

Sign up for GitHub

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

Jump to bottom

Real set variable #240

Closed
wants to merge 8 commits into from
Closed

Real set variable #240

wants to merge 8 commits into from

Conversation

Jeremi360
Copy link
Contributor

@Jeremi360 Jeremi360 commented May 26, 2023 •
edited
Loading

fixes #237

@Jeremi360 Jeremi360 requested a review from theludovyc May 26, 2023 15:26
@Jeremi360 Jeremi360 self-assigned this May 26, 2023
@Jeremi360 Jeremi360 mentioned this pull request May 26, 2023
Open
theludovyc
theludovyc requested changes May 28, 2023
View reviewed changes
Copy link
Collaborator

@theludovyc theludovyc left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not forget to update unit tests or add new like you say

Empty lines are here to make code more readable, why you remove them ?

addons/Rakugo/lib/systems/Parser.gd Outdated Show resolved Hide resolved
addons/Rakugo/lib/systems/Executer.gd Outdated Show resolved Hide resolved
@Jeremi360
Copy link
Contributor Author

Jeremi360 commented May 29, 2023 •
edited
Loading

Do not forget to update unit tests or add new like you say

I'm doing it, this is way I made this PR a draft in the first place.

Empty lines are here to make code more readable, why you remove them ?

Wow sorry dude, but you made new line so often that for me it looked bad, so I thought that some thing was wrong.
For me it was harder to read it. I thought it was some formatter or code prettyfier go crazy.
I will try to restore your empty lines.

@Jeremi360
Copy link
Contributor Author

I added tests, but my code doesn't work and I don't know way.
Can you help me @theludovyc ?

@theludovyc
Copy link
Collaborator

theludovyc commented Aug 7, 2023 •
edited
Loading

I not forgot you @Jeremi360 😊

@Jeremi360
Copy link
Contributor Author

Jeremi360 commented Aug 7, 2023 •
edited
Loading

Do not forget to update unit tests or add new like you say

Empty lines are here to make code more readable, why you remove them ?

I think I already restored your empty lines @theludovyc

@theludovyc
Copy link
Collaborator

@Jeremi360 I mean, to help you 😊

theludovyc
theludovyc requested changes Aug 8, 2023
View reviewed changes
addons/Rakugo/lib/systems/Executer.gd
Comment on lines +153 to +155
var var_ = null
if Rakugo.has_variable(var_name):
var_ = Rakugo.get_variable(var_name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rakugo.get_variable return null if variable is not found. So you do not need to use has_variable before get_variable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I made this if this way as Rakugo would crash when get_variable() returns null - we program it this way,
push_error() stops program no matter what is next.

Rakugo-Dialogue-System/addons/Rakugo/Rakugo.gd

Lines 74 to 85 in d11e1fe

func get_variable(var_name: String):
var vars_ = var_name.split(".")
match vars_.size():
1:
if store_manager.variables.has(var_name):
return store_manager.variables.get(var_name)
2:
return get_character_variable(vars_[0], vars_[1])
push_error("Rakugo does not knew a variable called: " + var_name)

addons/Rakugo/lib/systems/Parser.gd Outdated
Comment on lines 238 to 240
if str_expression.is_empty():
parse_array.push_back([key, result])
break
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If your str_expression is empty why you do not send an error ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, I last touch this code over a month ago, so maybe I was to change into something and forgot.
I will test with out this if-block and only if find that is really need restore it in proper form.

@theludovyc
Copy link
Collaborator

theludovyc commented Aug 8, 2023 •
edited
Loading

I added tests, but my code doesn't work and I don't know way. Can you help me @theludovyc ?

image

@Jeremi360 Do you try to debug with prints ?

@theludovyc
Copy link
Collaborator

theludovyc commented Aug 8, 2023 •
edited
Loading

@Jeremi360 You need to fix this bug #99, to fix yours

@Jeremi360
Copy link
Contributor Author

@theludovyc
About #240 (comment) :
I tried to debug it with unit-test and also with debug with prints and like on your screenshot it only catches strings.

About #99 I will try to fix it also then.

When I try to run any of RKScripts examples in Godot 4.1.1 it always start for this 3 errors:
image
So which should we use instead call_deferred() or call_thread_group() ?

@theludovyc
Copy link
Collaborator

theludovyc commented Aug 15, 2023 •
edited
Loading

I found why we have these errors now godotengine/godot#78214 (comment)

Edit : Fix is ready, I made a pr soon :) !

Edit : here #241

@Jeremi360
Copy link
Contributor Author

Jeremi360 commented Aug 17, 2023 •
edited
Loading

I found why we have these errors now godotengine/godot#78214 (comment)

Edit : Fix is ready, I made a pr soon :) !

Edit : here #241

@theludovyc Nice! I will merge it soon!

@Jeremi360
Copy link
Contributor Author

partialy implemented by #263

@Jeremi360 Jeremi360 closed this Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theludovyc theludovyc theludovyc requested changes

Assignees

@Jeremi360 Jeremi360

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SET_VARIABLE is almost just DEFINE_VARIABLE
2 participants
@Jeremi360 @theludovyc