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

Make the lightmap size hint property appear as read-only in the editor #77260

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented May 19, 2023

Lightmap Size Hint is set on import or when modifying a PrimitiveMesh's size, so manual changes to it are often lost.

However, being able to see it for debugging purposes is useful, so it's visible as a read-only property. This does not impact your ability to set lightmap_size_hint from a script (for procedural generation).

See discussion starting from #75164 (comment).

Preview

image

@akien-mga
Copy link
Member

I wonder if it's explicit enough to users why this property is read only. Usually read only properties are like this because they're not usable in a specific config, but they can become writeable in other conditions. Here it's always going to be read only but there are no explanations aside from the comment in the source code.

The documentation only says "Sets a hint to be used for lightmap resolution." which doesn't explain that it can't/shouldn't be set by the user. And the property and its setter are accessible from script so the value can still be changed.

@Calinou Calinou force-pushed the editor-lightmap-size-hint-read-only branch from a410584 to 13d780a Compare May 23, 2023 12:32
@Calinou Calinou requested a review from a team as a code owner May 23, 2023 12:32
@Calinou
Copy link
Member Author

Calinou commented May 23, 2023

I've improved documentation for the Mesh.lightmap_size_hint property.

Lightmap Size Hint is set on import or when modifying a
PrimitiveMesh's size, so manual changes to it are often lost.

However, being able to see it for debugging purposes is useful,
so it's visible as a read-only property.
@Calinou Calinou force-pushed the editor-lightmap-size-hint-read-only branch from 13d780a to ebebe3b Compare May 23, 2023 12:33
@BlueCube3310
Copy link
Contributor

Since #75164 the user has more control over an individual mesh instance's lightmap size. I think setting the hint to read-only makes more sense now

@michaelharmonart
Copy link

Although the lightmap size multiplier is nice, there are still use-cases for manually defining the lightmap size of an object in real units (pixels in this case)
godotengine/godot-proposals#11383

I wrote up the above proposal earlier this week specifically because I was running into the the issue of lightmap size hints being reset.

BUT we still need a way to generate better sane defaults for the lightmap hint on imported scenes based on mesh size. Currently large meshes that are impractical to run through Godot's UV2 generation are assigned stupidly small lightmap size hints that mean now you'd be turning the lightmap size scale not to 2x or even 4x but probably on the order of 32x to 64x in many cases.

Basically my point is: although the lightmap size hint SHOULDN'T need to be edited manually, and will often be reset, for me specifically it's the only way I've used successfully so far to get the result I need (actual decent texel resolution for large chunks of a landscape that have UV2s authored externally)

If we could get lightmap size hints to be set automatically based on the size of imported meshes that aren't run through the godot UV generation, that would be ideal. At that point I'd be 100% on board with this change.

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.

4 participants