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

Tweak StandardMaterial3D's default height properties #49947

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 27, 2021

  • Set the default height scale to 5.0 and multiply it by 0.01 in the shader.
  • Document heightmap properties in BaseMaterial3D class reference.

@Calinou Calinou added this to the 4.0 milestone Jun 27, 2021
@Calinou Calinou force-pushed the standardmaterial3d-tweak-default-height-properties branch from 86137b6 to c352376 Compare June 27, 2021 14:48
@Calinou Calinou requested a review from a team as a code owner June 27, 2021 14:48
@akien-mga akien-mga requested a review from a team June 28, 2021 13:06
@Calinou Calinou force-pushed the standardmaterial3d-tweak-default-height-properties branch from c352376 to 706dede Compare August 16, 2021 03:58
@reduz
Copy link
Member

reduz commented Aug 24, 2021

I dont think this is a great idea, if you are making a mobile game, deep parallax is very expensive and makes the technique unusable.

@Calinou Calinou force-pushed the standardmaterial3d-tweak-default-height-properties branch from 706dede to 391d64d Compare August 25, 2021 05:59
@Calinou
Copy link
Member Author

Calinou commented Aug 25, 2021

I dont think this is a great idea, if you are making a mobile game, deep parallax is very expensive and makes the technique unusable.

I amended the pull request to leave Deep Parallax disabled by default. Hopefully, #50383 should make non-deep parallax more usable on most materials.

@Calinou Calinou force-pushed the standardmaterial3d-tweak-default-height-properties branch from 391d64d to 53b4ac0 Compare November 16, 2021 14:24
@reduz
Copy link
Member

reduz commented Nov 16, 2021

Regarding this, what is generally the value range expected in similar applications/materials? I would think its the same spaces as the world.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

We discussed this in PR review meeting and agreed that it should be scaled by 0.01 (so if you interpret the unit as meter this property would be edited as cm, though this shouldn't be mentioned as is in the docs probably).

The scaling should be explained in the docs.

@Calinou
Copy link
Member Author

Calinou commented Aug 2, 2022

We discussed this in PR review meeting and agreed that it should be scaled by 0.01 (so if you interpret the unit as meter this property would be edited as cm, though this shouldn't be mentioned as is in the docs probably).

Does this mean the default height should be 5 now?

@akien-mga
Copy link
Member

Yes.

@Calinou Calinou force-pushed the standardmaterial3d-tweak-default-height-properties branch 2 times, most recently from d40ddfd to b339bbd Compare August 4, 2022 00:03
@Calinou
Copy link
Member Author

Calinou commented Aug 4, 2022

Rebased and tested again, with the default height changed to 5.0. I also filled in the documentation for all heightmap properties in BaseMaterial3D. I think the note on heightmap normalization will be helpful in particular, as it's something I've found to improve the effect in many cases 🙂

We could add an import option to normalize an image, so that you don't need to use an image editor for this.

@akien-mga
Copy link
Member

akien-mga commented Aug 4, 2022

You need to convert the > in XML.
We should maybe add validation for this to makerst so that the precommit hook can catch it.

- Set the default height scale to 5.0 and multiply it by 0.01 in the shader.
- Document heightmap properties in BaseMaterial3D class reference.
@Calinou Calinou force-pushed the standardmaterial3d-tweak-default-height-properties branch from b339bbd to c6cca43 Compare August 4, 2022 07:58
@akien-mga akien-mga merged commit 40eafcf into godotengine:master Aug 4, 2022
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou deleted the standardmaterial3d-tweak-default-height-properties branch August 4, 2022 09:32
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.

3 participants