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

Add support for generating noise images with an offset #48805

Merged
merged 1 commit into from
May 20, 2021
Merged

Add support for generating noise images with an offset #48805

merged 1 commit into from
May 20, 2021

Conversation

radishes
Copy link
Contributor

Problem
OpenSimplexNoise.get_image() and NoiseTexture can only generate noise from coordinates in the range (0,0) to (image_width, image_height). The result is that these classes can't be used to generate texture data across a larger space. My use case is that I want to generate noise data at arbitrary coordinates so that I can use the data for creating a procedural world, and passing to shaders.

Workaround
It is possible to achieve the desired outcome in gdscript. I can generate an image by calling OpenSimplexNoise.get_noise_2d() for each coordinate, and generate a texture from that image. However, doing this in gdscript is very slow.

Solution
In this PR, I've updated OpenSimplexNoise.get_image() and NoiseTexture to accept a coordinate offset, so that they can generate noise images from anywhere in the noise space. In my tests, using the updated method is about an order of magnitude faster than using the gdscript method mentioned above.

Rationale

  • This enhancement enables the generation of large amounts of noise data.
  • This enhancement is about 10x faster than doing the same thing in gdscript.
  • This enhancement does not break compatibility.
  • This enhancement is small and easily understood.

@radishes radishes requested a review from a team as a code owner May 18, 2021 06:36
@radishes
Copy link
Contributor Author

radishes commented May 18, 2021

Demo
Create a Node2D and attach the following script.

The default behavior is to generate a grid of sprites containing noise data using gdscript. This is the slow method which is compatible with Godot 3.3.

Running generate_noise_texture_grid() or generate_image_texture_grid() instead of generate_manually_offset_grid() will demonstrate the new, faster functionality provided by this PR.

extends Node2D

var grid_size = 4
var texture_size = 512
var noise = OpenSimplexNoise.new()

func _ready():
	generate_manually_offset_grid()
	#generate_noise_texture_grid()
	#generate_image_texture_grid()

func generate_manually_offset_grid():
	"""Generate noise textures by manually calculating offset in gdscript."""
	for grid_y in range(grid_size):
		for grid_x in range(grid_size):
			var pos = Vector2(grid_x, grid_y) * texture_size
			var data := PoolByteArray()
			data.resize(texture_size * texture_size)
			for y in range(texture_size):
				for x in range(texture_size):
					var n = noise.get_noise_2d(x + pos.x, y + pos.y) * 0.5 + 0.5
					data[y * texture_size + x] = n * 256
			var image := Image.new()
			image.create_from_data(texture_size, texture_size, false, Image.FORMAT_L8, data)
			var texture := ImageTexture.new()
			texture.create_from_image(image, 0)
			var sprite := Sprite.new()
			sprite.texture = texture
			sprite.position = pos
			add_child(sprite)

func generate_noise_texture_grid():
	"""Generate NoiseTextures using new offset feature."""
	for y in range(grid_size):
		for x in range(grid_size):
			var noise_texture := NoiseTexture.new()
			noise_texture.noise = noise
			noise_texture.width = texture_size
			noise_texture.height = texture_size
			var pos = Vector2(texture_size * x, texture_size * y)
			noise_texture.noise_offset = pos
			var sprite = Sprite.new()
			sprite.position = pos
			sprite.texture = noise_texture
			add_child(sprite)

func generate_image_texture_grid():
	"""Generate noise images using new offset feature and create texture manually."""
	for y in range(grid_size):
		for x in range(grid_size):
			var sprite = Sprite.new()
			var pos = Vector2(texture_size * x, texture_size * y)
			sprite.position = pos
			var image
			image = noise.get_image(texture_size, texture_size, pos)
			var tex = ImageTexture.new()
			tex.create_from_image(image, 0)
			sprite.texture = tex
			add_child(sprite)

@clayjohn
Copy link
Member

This change would also have the side effect of fixing #47040

@clayjohn clayjohn requested a review from JFonS May 18, 2021 14:25
@radishes
Copy link
Contributor Author

This has been tagged "breaks compat" but I don't believe it does break compatibility. The default behavior remains the same as before.

@radishes
Copy link
Contributor Author

This change would also have the side effect of fixing #47040

It wouldn't fix #47040 per se, as the (0, 0) noise space pixel would be generated as before, and because the default behavior is still to generate a noise image which spans (0, 0) - (width, height). But this enhancement would give users more flexibility in working around #47040.

@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release and removed breaks compat labels May 18, 2021
modules/opensimplex/doc_classes/NoiseTexture.xml Outdated Show resolved Hide resolved
modules/opensimplex/doc_classes/NoiseTexture.xml Outdated Show resolved Hide resolved
modules/opensimplex/doc_classes/OpenSimplexNoise.xml Outdated Show resolved Hide resolved
modules/opensimplex/doc_classes/OpenSimplexNoise.xml Outdated Show resolved Hide resolved
@radishes radishes requested review from Calinou and removed request for a team May 18, 2021 16:26
@radishes radishes requested a review from Calinou May 18, 2021 16:43
@radishes radishes requested a review from Calinou May 18, 2021 16:53
@radishes
Copy link
Contributor Author

This change would also have the side effect of fixing #47040

It wouldn't fix #47040 per se, as the (0, 0) noise space pixel would be generated as before, and because the default behavior is still to generate a noise image which spans (0, 0) - (width, height). But this enhancement would give users more flexibility in working around #47040.

At the suggestion of @Xrayez I updated this PR to use Vector2 all the way through. This gives an effective way to work around #47040: pass non-integer offsets, for example (0.5, 0.5), and the noise map will not generate a value for (0, 0).

@radishes radishes requested a review from Xrayez May 19, 2021 16:14
@clayjohn clayjohn removed the request for review from JFonS May 20, 2021 05:03
@clayjohn
Copy link
Member

Looks great! Can you please squash your commits into one. We like to keep our git history very clean. After that, it should be ready to merge!

@radishes
Copy link
Contributor Author

Looks great! Can you please squash your commits into one. We like to keep our git history very clean. After that, it should be ready to merge!

Done. Thank you for the link!

@akien-mga
Copy link
Member

FYI, your local Git author details (see header of https://github.com/godotengine/godot/commit/f33800a9c8023843de271280a7482a5b7911e8e6.patch) don't seem to match the email registered in your GitHub account, which is why the commit doesn't appear as authored by "@radishes", so your GitHub account would not be properly credited as contributor once merged. It's not a problem technically for Git or GitHub, but if you want to claim this commit to your account, you can add this email as secondary email in your GitHub settings.

@radishes
Copy link
Contributor Author

FYI, your local Git author details (see header of https://github.com/godotengine/godot/commit/f33800a9c8023843de271280a7482a5b7911e8e6.patch) don't seem to match the email registered in your GitHub account, which is why the commit doesn't appear as authored by "@radishes", so your GitHub account would not be properly credited as contributor once merged. It's not a problem technically for Git or GitHub, but if you want to claim this commit to your account, you can add this email as secondary email in your GitHub settings.

Thank you! I've added that email address as a secondary email to my Github account.

Copy link
Contributor

@JFonS JFonS left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

@akien-mga akien-mga merged commit 78d85de into godotengine:master May 20, 2021
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga
Copy link
Member

akien-mga commented May 21, 2021

This was labeled as a candidate to cherry-pick for 3.x, but there's an issue due to #30424 which was not cherry-picked as it breaks compatibility. So I'm not sure if adding this feature as is would make sense or if the x and y coordinates should also be swapped, but that potentially makes things even more confusing.

@radishes
Copy link
Contributor Author

I think swapping the offset coordinates to match the bug is the least surprising outcome for users. The example below produces tile-able continuous noise. Maybe an explanatory comment would help?

float v = get_noise_2d(float(i) + p_noise_offset.y, float(j) + p_noise_offset.x); // Axes swapped to maintain compatibility, per #30424.

@Xrayez
Copy link
Contributor

Xrayez commented May 21, 2021

In any case, #30424 is a clear bug to me, and the PR which could've been cherry-picked in 3.1 as well, but I guess too many people might already rely on the wrongly swapped coordinates by now in 3.3...

@radishes
Copy link
Contributor Author

If there's going to be a 3.4, maybe that's a good time to break compatibility and fix the bug? Is that option on the table?

@Xrayez
Copy link
Contributor

Xrayez commented May 21, 2021

If there's going to be a 3.4, maybe that's a good time to break compatibility and fix the bug? Is that option on the table?

I personally think it would be worth it, as long as the change is documented of course. It feels wrong to propagate the bug with workarounds further. But it's up to Akien to decide.

It's probably a change which is easy to recover in existing projects. It would mostly affect procedural generation, and most of the time you don't really care about this sort of thing for randomness, unless you rely on reproducibility of the procedurally generated levels, but that's a quite specific use case. In fact, you shouldn't rely on reproducible results across platforms with Godot, a lot of the times it's just implementation detail. 😛

We've had regressions in NoiseTexture due to major thread modernization backports in the past: #46907 (and many other regressions because of #45618), so I don't see a reason why such a minor bug fix cannot be cherry-picked...

@akien-mga
Copy link
Member

I agree, I cherry-picked #30424 for 3.4.

We've had regressions in NoiseTexture due to major thread modernization backports in the past: #46907 (and many other regressions because of #45618), so I don't see a reason why such a minor bug fix cannot be cherry-picked...

That's not a very good argument. Those regressions have been fixed before the stable release, so there's no compat breakage in that specific instance. On the other hand, the now cherry-picked #30424 will break compat in a stable release. That's apples and oranges.

@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label May 31, 2021
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.

6 participants