Skip to content

Conversation

@wjt
Copy link
Member

@wjt wjt commented Oct 22, 2024

This is an alternative to:

with backwards compatibility with existing block programs. It roughly implements @dbnicholson's suggestion in #272 (review):

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.

  1. Keep an unexported spawn_frequency that wraps spawn_period with getters and setters.

Actually it has to be exported; otherwise values in existing .tscn files are not set. I learned about the @export_storage annotation, added in Godot 4.3, which does what we want here. However that does mean bumping the minimum supported Godot version. And also it means that both properties will be saved in scenes – not only existing scenes which instantiated SimpleSpawner and set the old property, but also those that set the new name.

  1. 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 hidden block definition attribute. Or you could add an aliases block definition attribute that was used for lookup when loading.

I took the hidden path. But having written it, I think the result is kind of ugly. Maybe aliases would be better.

It was interesting to learn some things about properties, but having written all this I kind of think we should just go with #272: make the destructive change and call the next release 0.8.0. I'm interested in other opinions though.

wjt added 3 commits October 14, 2024 11:03
I learned about this annotation in a workshop hosted by Nathan from GDQuest.
This makes it possible to define a block that cannot be added to new block
programs, but which will still work if present in a block program.

The intended use is to fix SimpleSpawner's "spawn frequency" property and
blocks, which is in fact the spawn period (frequency is the reciprocal of
period).
@wjt wjt force-pushed the simplespawner-frequency-is-actually-period-bis branch from da29337 to bc93c4f Compare October 22, 2024 14:53
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.

Add a new property and corresponding blocks with the correct definition.

Adjust the existing property to delegate to the new property. Use
@export_storage to allow its value to be set from existing serialized scenes
without it being visible in the inspector. This annotation is new in Godot 4.3
so bump our minimum supported version.

When a scene that overrides the old property is loaded in the editor, the value
will be propagated to the new property, which is then saved into the scene.
Adjusting the new property also adjusts the old property's value. The old
property will hang around, harmlessly mirroring the new property, in the .tscn
file until it is removed in a text editor.

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 wjt force-pushed the simplespawner-frequency-is-actually-period-bis branch from bc93c4f to 17d897e Compare October 22, 2024 14:54
@wjt
Copy link
Member Author

wjt commented Oct 22, 2024

I'm trying a third version with the ability to alias blocks...

@dbnicholson
Copy link
Member

  1. Keep an unexported spawn_frequency that wraps spawn_period with getters and setters.

Actually it has to be exported; otherwise values in existing .tscn files are not set. I learned about the @export_storage annotation, added in Godot 4.3, which does what we want here. However that does mean bumping the minimum supported Godot version. And also it means that both properties will be saved in scenes – not only existing scenes which instantiated SimpleSpawner and set the old property, but also those that set the new name.

Huh, I thought I tried to do that in a test project and it worked. Testing again, I see it doesn't. Now I'm not nearly as ambitious about handling this.

@wjt
Copy link
Member Author

wjt commented Oct 23, 2024

I tried adding an aliases property to BlockDefinition. After a bit of plumbing, it works correctly. But we still need to add the backwards-compat alias to the property itself (or else the spawn frequency property block, which is used in the sample scene, stops working); and we also need a way to alias block parameters, because the current set spawn frequency block is defined thusly:

set spawn frequency to {new_frequency: FLOAT}

so if I change this to:

set spawn period to {new_period: FLOAT}

then even with an alias so the block is still found, its parameter gets lost.

I really don't have the appetite for adding named parameter aliases.

Anyway, I'll push my block alias branch for posterity:

but I'm not going to work on it further. I think we should either go with this one, or (my preference) #272.

@dbnicholson
Copy link
Member

I really don't have the appetite for adding named parameter aliases.

Indeed, let's just carry on with this. Ultimately I think all of this needs to be handled by adding some kind of scene migration. Unfortunately, I don't know how you'd do that exactly.

@wjt wjt closed this Oct 24, 2024
@wjt wjt deleted the simplespawner-frequency-is-actually-period-bis branch October 24, 2024 06:29
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