Skip to content

Rename Perlin noise to (fractal) value noise#15858

Merged
sfan5 merged 1 commit into
luanti-org:masterfrom
kno10:rename-perlin
Apr 10, 2025
Merged

Rename Perlin noise to (fractal) value noise#15858
sfan5 merged 1 commit into
luanti-org:masterfrom
kno10:rename-perlin

Conversation

@kno10

@kno10 kno10 commented Mar 1, 2025

Copy link
Copy Markdown
Contributor

Fixes #15775: rename Perlin noise to (fractal) value noise, which is what is actually implemented.

I tried to add aliases for the old names. But I'm not sure what is the best way to generate a deprecation warning for the global lua classes such as PerlinNoise and PerlinNoiseMap, don't want to break anything. But these old names are probably not going to be removed the next 5+ years anyway...
This needs to be fine-tuned by an expert on the Luanti Lua API.

In the future, actual Perlin noise or simplex noise could then be added for new mapgens.
For example the fast https://github.com/Auburn/FastNoise2 library is C++ and MIT licensed.

See also:

@appgurueu appgurueu changed the title Rename Perlin noise to (fractal) value noise [no sq] Rename Perlin noise to (fractal) value noise Mar 13, 2025
Comment thread doc/lua_api.md Outdated

Perlin noise creates a continuously-varying value depending on the input values.
Value noise creates a continuously-varying value depending on the input values,
it is a simpler (but faster) version of Perlin noise.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I suggest turning this into two sentences:

Value noise creates a continuously-varying value depending on the input values.
It is similar to Perlin noise, but interpolates between random values instead of
gradient vectors.
  1. both 'simple' and 'fast' generally have positive connotations, so negating one over the other using 'but' reads unnatural
  2. I'd argue that performance is not a property of the noise itself, but of its implementation, even though a noise itself can be designed to be easier on compute. keep in mind I'm not an expert though :p

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Performance is supposedly higher, because interpolating between values is simpler than interpolating gradients.
But it will likely be a small constant. In the FastNoiseListe benchmarks, 2D value noise is about 16-24% faster than Perlin noise across three implementations.

Comment thread src/script/lua_api/l_env.cpp Outdated
@sfence sfence added the Breaking Change Changes an existing API behavior in an incompatible way label Mar 22, 2025
@wmikita

wmikita commented Mar 23, 2025

Copy link
Copy Markdown

@sfence this specific PR does not break any API compatibility, it simply introduces a deprecation due to a rename

@Zughy Zughy removed the Breaking Change Changes an existing API behavior in an incompatible way label Mar 23, 2025
@sfence

sfence commented Mar 23, 2025

Copy link
Copy Markdown
Contributor

@wmikita Valid point for get_perli, not for PerlinNoise if I see well.

@wmikita

wmikita commented Mar 24, 2025

Copy link
Copy Markdown

@wmikita Valid point for get_perli, not for PerlinNoise if I see well.

global Lua-facing classes aren't touched here yet. the idea is to generate a deprecation warning when somebody tries to use them, similarly to get_perlin and get_perlin_map

@sfence

sfence commented Mar 24, 2025

Copy link
Copy Markdown
Contributor

@wmikita My error, I missed line lua_register(L, "PerlinNoise", create_object); // deprecated name

Comment thread src/script/lua_api/l_noise.cpp Outdated
Comment thread src/script/lua_api/l_noise.cpp Outdated
@grorp

grorp commented Mar 24, 2025

Copy link
Copy Markdown
Member

As far as I can see:

  • You're planning to add perlin and simplex noise in a follow-up PR: Add Perlin and Simplex noise #15877
  • You're planning to have them use the same API (that actually seems like a good idea to me, can avoid code and "concept" duplication)

So why are you renaming these APIs to "value noise" here then? That will be obsolete again with your follow-up PR.

@grorp grorp added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Action / change needed Code still needs changes (PR) / more information requested (Issues) @ Script API labels Mar 24, 2025
@kno10

kno10 commented Mar 25, 2025

Copy link
Copy Markdown
Contributor Author

@grorp:

You're planning to add perlin and simplex noise in a follow-up PR: Add Perlin and Simplex noise #15877

I did not plan that when I created this pull request.

You're planning to have them use the same API (that actually seems like a good idea to me, can avoid code and "concept" duplication)

It's ugly and limiting though. Only a subset of the FastNoiseLite library currently used is available this way.
I am all but convinced that this is the best way to proceed. A new mapgen may need to design a new noise API to better make use of additional features.

The integration into the old noise API primarily serves the purpose that people can see the difference between the noise quality generated for an actual mapgen, in-game.

So why are you renaming these APIs to "value noise" here then? That will be obsolete again with your follow-up PR.

It will be only obsolete if that second pull is merged as is, and we do not end up making a much better noise API.
ValueNoise is a decent name for the old API; and as stand-alone pull request, this still makes sense.

@grorp grorp removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 27, 2025
@grorp

grorp commented Mar 27, 2025

Copy link
Copy Markdown
Member

I did not plan that when I created this pull request.
[...]
It will be only obsolete if that second pull is merged as is, and we do not end up making a much better noise API.

I suppose what you'll have to figure out is: what are the plans for these noise APIs and your second PR? Will a separate general noise API actually happen (this rename would then make sense), or will the current API become the general one (should choose a different name then)?

The reason to think about this now is that we shouldn't end up renaming and still having an inaccurate name, or even renaming twice.

@kno10

kno10 commented Mar 27, 2025

Copy link
Copy Markdown
Contributor Author

@grorp I don't think I would suggest to do it the way the second pull hacks noise into the existing API via flags.
Hence this pull here makes sense: we won't be able to get rid of the old noise for compatibility, but it is not Perlin.

@sfence sfence left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks fine, and works as expected.

Comment thread builtin/emerge/env.lua Outdated
@sfan5 sfan5 added Action / change needed Code still needs changes (PR) / more information requested (Issues) Rebase needed The PR needs to be rebased by its author labels Apr 3, 2025
@kno10 kno10 changed the title [no sq] Rename Perlin noise to (fractal) value noise Rename Perlin noise to (fractal) value noise Apr 5, 2025
@kno10 kno10 requested a review from sfan5 April 5, 2025 20:17
core.get_perlin = core.get_value_noise
core.get_perlin_map = core.get_value_noise_map
PerlinNoise = ValueNoise
PerlinNoiseMap = ValueNoiseMap

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this code block in deprecated.lua and env.lua at the same time?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I understood the emerge/env to only apply to the voxel manipulator emerge context, whereas builtin/game seemed to apply to the entire game context.
Maybe this could be better documented.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, this is fine.

@Zughy Zughy removed Rebase needed The PR needs to be rebased by its author Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Apr 6, 2025

@appgurueu appgurueu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i have some minor suggestions, but nothing which really needs something to be done about it.

the reason i originally wanted this to be no squash is because while the C++ renames should be pretty uncontroversial 1, the Lua api deprecations are trickier, so i wanted that in a separate commit. but now that the warnings are gone it's fine.

the "new" aliases don't need to support the deprecated multi-param calls (but i can see why it might be more convenient to keep them, so 🤷)

the docs should probably mention that the new aliases are 5.12+? 2

Footnotes

  1. why no mention of the fact that it's value noise in the C++ names? i suppose this is to facilitate adding perlin noise later?

  2. not sure if we have a policy on this. in principle modders should use the docs corresponding to the luanti version they are targeting, but we typically include such version numbers nevertheless

Comment thread src/noise.cpp
Comment thread doc/lua_api.md Outdated
@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Apr 8, 2025
@sfan5 sfan5 removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements @ Mapgen @ Script API >= Two approvals ✅ ✅

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Perlin noise implementation is not Perlin noise (but value noise)

8 participants