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 NoiseTexture3D and rename NoiseTexture to NoiseTexture2D. #64660

Merged
merged 0 commits into from
Aug 22, 2022
Merged

Add NoiseTexture3D and rename NoiseTexture to NoiseTexture2D. #64660

merged 0 commits into from
Aug 22, 2022

Conversation

Lasuch69
Copy link
Contributor

@Lasuch69 Lasuch69 commented Aug 20, 2022

Add NoiseTexture3D and rename NoiseTexture to NoiseTexture2D.

This commit implements new resource NoiseTexture3D. Unfortunately it doesn't support seamless generation.
Note: Mipmaps are buggy as some resolutions (only lower than default) break format required size and can't intialize. Also if you change resolution too fast you get a size mismatch error.
As for Noise resource modification i only changed arguments for generation to support custom depth allowing for 3D coordinates to be used.

modified:   modules/noise/config.py
modified:   modules/noise/doc_classes/Noise.xml
renamed:    modules/noise/doc_classes/NoiseTexture.xml -> modules/noise/doc_classes/NoiseTexture2D.xml
new file:   modules/noise/doc_classes/NoiseTexture3D.xml
modified:   modules/noise/editor/noise_editor_plugin.cpp
modified:   modules/noise/noise.cpp
modified:   modules/noise/noise.h
renamed:    modules/noise/noise_texture.cpp -> modules/noise/noise_texture_2d.cpp
renamed:    modules/noise/noise_texture.h -> modules/noise/noise_texture_2d.h
new file:   modules/noise/noise_texture_3d.cpp
new file:   modules/noise/noise_texture_3d.h
modified:   modules/noise/register_types.cpp

Bugsquad edit: This closes godotengine/godot-proposals#4946.

@Lasuch69 Lasuch69 requested a review from a team as a code owner August 20, 2022 16:35
@Lasuch69 Lasuch69 marked this pull request as draft August 20, 2022 17:29
@Lasuch69 Lasuch69 marked this pull request as ready for review August 20, 2022 17:30
@Geometror
Copy link
Member

Thanks for your work on this :)
First things first: It is recommended that you never use your master branch for pull requests. While it essentially works for now, you can quickly run into problems this way.
Your pull request description contains information about which files were modified/added/removed, which is redundant since the GitHub UI already shows this. It would be more helpful to provide a short summary about what your PR does (although more detailed than the title)/what remains to do or which problems you faced.
On a first glance the implementation looks quite good to me, but I need to review it more thoroughly.

@Lasuch69
Copy link
Contributor Author

Thanks for your work on this :) First things first: It is recommended that you never use your master branch for pull requests. While it essentially works for now, you can quickly run into problems this way. Your pull request description contains information about which files were modified/added/removed, which is redundant since the GitHub UI already shows this. It would be more helpful to provide a short summary about what your PR does (although more detailed than the title)/what remains to do or which problems you faced. On a first glance the implementation looks quite good to me, but I need to review it more thoroughly.

I will try to improve on that. Thanks for advice!

@Lasuch69 Lasuch69 marked this pull request as draft August 20, 2022 17:44
@Geometror
Copy link
Member

In addition to my comment before,
https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html provides a very helpful and detailed explanation of the Godot pull request workflow.

@aaronfranke aaronfranke added this to the 4.x milestone Aug 20, 2022
@Lasuch69 Lasuch69 marked this pull request as ready for review August 20, 2022 18:44
@Lasuch69
Copy link
Contributor Author

I needed to fix a couple of typos and add comments

@Lasuch69
Copy link
Contributor Author

updated fork

@Lasuch69 Lasuch69 merged commit baabad3 into godotengine:master Aug 22, 2022
@Lasuch69
Copy link
Contributor Author

Lasuch69 commented Aug 22, 2022

i hate my life

rebase conflict defeated me

i will try to improve it as much as possible and i give up for now with PR's

@Calinou
Copy link
Member

Calinou commented Oct 26, 2022

i hate my life

rebase conflict defeated me

i will try to improve it as much as possible and i give up for now with PR's

Do you still have the code somewhere? This PR no longer includes any code, which makes it impossible for other people to salvage your work (if you no longer have time to work on it).

@Lasuch69
Copy link
Contributor Author

Lasuch69 commented Oct 29, 2022

Do you still have the code somewhere? This PR no longer includes any code, which makes it impossible for other people to salvage your work (if you no longer have time to work on it).

I do. Here: https://github.com/Lasuch69/add_noisetexture3d/tree/add-noisetexture3d The problem is this function: https://github.com/Lasuch69/add_noisetexture3d/blob/cdf5a02c276165c27ad7a97d716b14d7a4106c47/drivers/vulkan/rendering_device_vulkan.cpp#L1147 on "for" loop iterates on mipmap levels (u_int) and sometimes it doesn't take into account lowest layers for example 1x1x4 is 7 pixels in total with mipmaps and godot only sees 4 because this function iterates only through layer 0. Layers are created by width and height only and depth isn't taken into account. Tbh i don't know what to do so feel free to do anything with this code you want.

@akien-mga akien-mga removed this from the 4.x milestone Dec 7, 2022
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.

Add NoiseTexture3D
5 participants