Skip to content

completed-signal for coroutines with more than one yield#17291

Merged
hpvb merged 1 commit into
godotengine:masterfrom
Warlaan:master
Mar 15, 2018
Merged

completed-signal for coroutines with more than one yield#17291
hpvb merged 1 commit into
godotengine:masterfrom
Warlaan:master

Conversation

@Warlaan
Copy link
Copy Markdown
Contributor

@Warlaan Warlaan commented Mar 5, 2018

This is a proposal in response to issue #17280.
You can yield for completion of a coroutine like this: yield(coroutine(), "completed")
but without this PR that only works for coroutines with one yield inside, since every yield keyword returns a GDScriptFunctionState and the completed-signal was only emitted for the last one.

NOTE: there's probably something missing yet - it causes an error that I don't quite understand (Object::disconnect warns that the signal "completed" didn't exist) and I assume that referencing the previous GDScriptFunctionStates with raw pointers may not be a good idea since it probably won't make sure that the previous states aren't freed before their completed-signal is emitted.

@Warlaan
Copy link
Copy Markdown
Contributor Author

Warlaan commented Mar 5, 2018

The error was probably related to me not storing references before. That's fixed now, but I'd still appreciate it if someone had a look and told me if I am handling the references correctly.

@vnen vnen added this to the 3.1 milestone Mar 6, 2018
Comment thread modules/gdscript/gdscript_function.h Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Godot's C++ code uses snake_case for all identifiers, should be previous_state.

@akien-mga
Copy link
Copy Markdown
Member

Could you squash the commits together? Commits 2 and 3 are fixups of the first one, so we only need the final state.

…ine now, allowing to yield for completion of a function with more than one yield inside.
@Warlaan
Copy link
Copy Markdown
Contributor Author

Warlaan commented Mar 14, 2018

If it's ok I would use the opportunity to remove the code duplication in that place.

@akien-mga
Copy link
Copy Markdown
Member

If it's ok I would use the opportunity to remove the code duplication in that place.

Yeah that would be nice :)

@hpvb hpvb merged commit aed2fed into godotengine:master Mar 15, 2018
@hpvb
Copy link
Copy Markdown
Member

hpvb commented Mar 15, 2018

Tested, the code makes sense, and I think the struggle is real :)

Thanks!

@Douglas-W-Williams
Copy link
Copy Markdown
Contributor

@hpvb Any chance this will be added in 3.0.3?

@hpvb
Copy link
Copy Markdown
Member

hpvb commented Apr 14, 2018

Cherry picked into 3.0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants