SimpleSpawner: Rename frequency to period #272
Conversation
I learned about this annotation in a workshop hosted by Nathan from GDQuest.
dbnicholson
left a comment
dbnicholson
left a comment
There was a problem hiding this comment.
I agree in principle with this change completely even though I doubt learners will have the nuance to differentiate the terms or even know what they are. That said, I think we do want to use the correct terms.
An issue here is backwards compatibility. Changing the exported variable and block definition names will break any scene using SimpleSpawner (as implied by the changes in the example spawner scene). While we have not committed to any backwards compatibility, I feel like we shouldn't put this in a 0.7 patch release.
I think it would be possible to maintain compatibility in a couple ways.
- Keep an unexported
spawn_frequencythat wrapsspawn_periodwith getters and setters. - Duplicate the block definitions with the old names. However, that would make both blocks show up in the picker. You'd probably need to add some type of
hiddenblock definition attribute. Or you could add analiasesblock definition attribute that was used for lookup when loading.
I'm not sure we need to bother, but I wanted to discuss before merging.
|
Yes I think it's worth discussing a migration path. The alternatives 1. and 2. don't sound to me a proper migration, but more an effort to keep backwards compat with some overhead. Still we are considering this plugin as experimental and in early stages (as said in the README). So I'd say let's merge this for now. |
|
We came to the conclusion in discussion today that we should instead remove this demo because the new one from godotcon is a better demo of how to make a runner. |
|
@wjt maybe you wanted to close endlessm/godot-block-coding-demos#13 instead? |
|
Oops, yes I did. Well noticed! |
|
I think I'll take a quick look at compatibility.. |
This property is the time, in seconds, to wait between spawning scenes. This is a period/interval/wavelength, not a frequency, which would be measured in Hz, or 1/seconds. Adjust the name of the property and its documentation. Remove the unnecessary do_set_spawn_frequency() method in favour of generating a property assignment. Update the example scene accordingly. The instances of “frequency” that I have not changed are in the example scene's on-screen documentation. In this case it was actually used correctly: the keyboard action to increase the frequency reduces the property which is now called period, and the decrease action respectively increases the period.
wjt
force-pushed
the
simplespawner-frequency-is-actually-period
branch
from
5857e1c to
a353af4
Compare
|
Refreshed this PR to remove a redundant function. |
|
Let's just do this and ignore the compatibility for now. |
This property is the time, in seconds, to wait between spawning scenes. This is a period/interval/wavelength, not a frequency, which would be measured in Hz, or 1/seconds.
Adjust the name of the property and its documentation.
The instances of “frequency” that I have not changed are in the example scene's on-screen documentation. In this case it was actually used correctly: the keyboard action to increase the frequency reduces the property which is now called period, and the decrease action respectively increases the period.
I also added an annotation I learned yesterday to put units in the inspector slider for spawn period and spawn limit.