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 sound and particlespawner id generation #14059

Merged
merged 2 commits into from Nov 30, 2023

Conversation

Desour
Copy link
Member

@Desour Desour commented Nov 30, 2023

#14045 caused some issues:

With this PR, ids always grow, and don't return the last returned id. This is important, because the last returned id is likely to still be kept in lua.

cc @SmallJoker

To do

This PR is a Ready for Review.

How to test

It always returned 0.
Also, now the ids always grow, to make a conflict with ids in lua unlikely.
@Desour Desour added Blocker The issue needs to be addressed before the next release. Bugfix 🐛 PRs that fix a bug labels Nov 30, 2023
@Desour Desour added this to the 5.8.0 milestone Nov 30, 2023
@SmallJoker
Copy link
Member

Now that you properly fixed it, I now can see what was meant with

This cloud is "smooth" and has no obvious gaps.

The cloud did move around without gaps, but was much shorter as compared to the proper fix. I should have double-checked (e.g. printed) the generated ID's to make sure that my PR actually works instead of relying on a visual test.

Tested this PR with the following change to the original script in #14044:

		print(minetest.add_particlespawner({
				time = 1,
				amount = 50,
				minexptime = 0.5,
				maxexptime = 0.5,
				minpos = vector.offset(targ, -0.5, -0.5, -0.5),
				maxpos = vector.offset(targ, 0.5, 0.5, 0.5),
				minsize = 1,
				maxsize = 1,
				texture = "[combine:1x1^[noalpha",
			}), "black")
	end
...
				maxsize = 1,
				texture = "[combine:1x1^[noalpha^[invert:rgb",
			})
		print(oldid, "white")
	end
	minetest.after(0, fx)

Producing the output

1	white
2	black
3	white
4	white
5	black
6	white
7	white
8	white
9	black
10	white

Which produces the numbers as desired. Thank you very much for catching this issue so early.

@Zughy
Copy link
Member

Zughy commented Nov 30, 2023

@SmallJoker is your comment intended as an approval?

@Desour Desour merged commit 6106e4e into minetest:master Nov 30, 2023
13 checks passed
@Desour Desour deleted the fix_soundid_strictmonogrowth branch November 30, 2023 23:09
@SmallJoker
Copy link
Member

@Zughy I intended to wait so that someone else could double-check it for sanity - after all I was too quick merging mine, so I did not want that to happen again.

cx384 pushed a commit to cx384/minetest that referenced this pull request Dec 9, 2023
* Fix server sound ids being reused to early

* Fix particlespawner id generation

It always returned 0.
Also, now the ids always grow, to make a conflict with ids in lua unlikely.
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
* Fix server sound ids being reused to early

* Fix particlespawner id generation

It always returned 0.
Also, now the ids always grow, to make a conflict with ids in lua unlikely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocker The issue needs to be addressed before the next release. Bugfix 🐛 PRs that fix a bug One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants