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

Moves responsibility of checking texture compatibility to developer. #295

Merged
merged 3 commits into from
Jan 7, 2024

Conversation

lfxu
Copy link
Contributor

@lfxu lfxu commented Jan 6, 2024

Redoing #294. When setting a "New ImageTexture", a valid image of FORMAT_MAX is created. Now logs an error "Expects an actual texture file. See documentation for format suggestions."

Copy link
Owner

@TokisanGames TokisanGames left a comment

Choose a reason for hiding this comment

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

Thanks for putting this in another branch. This fixes the crashing problem. This issue remains:

  • When inserting a non-DXT5/RGBA8 image, then adding a subsequent texture, the texturing breaks when the new texture slot is created because the system generates a texture with DXT5/RGBA w/ mipmaps. The new texture already matches the previous texture size. It also needs to match the format.

It also generates these messages, but the cause is above

 servers/rendering/renderer_rd/storage_rd/texture_storage.cpp:884 - Condition "p_layers[i]->get_format() != valid_format" is true.
  Attempting to use an uninitialized RID

@lfxu
Copy link
Contributor Author

lfxu commented Jan 6, 2024

I did some investigation. The actual cause is:

  1. Clicking the plus sign to add a texture triggers _update_texture_data() without triggering _is_texture_valid().
  2. alb_tex.is_valid() is false in _update_texture_data(), but only on the first call. If you add another texture the second texture alb_tex.is_valid() is true. This shows the checkerboard texture is created after _update_texture_data() is called.

@TokisanGames
Copy link
Owner

TokisanGames commented Jan 6, 2024

We want it to generate a checkerbox texture when creating a new texture slot so that the array works. It just needs to match the format of anything that is already there. Or DXT5 if nothing. Change the format here, and in the similar section for the normal map:

https://github.com/TokisanGames/Terrain3D/blob/main/src/terrain_3d_texture_list.cpp#L109-L113

@lfxu
Copy link
Contributor Author

lfxu commented Jan 6, 2024

Thanks for clarifying. By "the system generates" I thought Godot did that.

Looks like there is not a straightforward way to convert an image to a specified format. One will have to deal with CompressMode etc. I also found godotengine/godot#79932. At this point, it feels not worth it to handle all the formats. Creating a blank texture as a fallback is always available though.

@TokisanGames
Copy link
Owner

This only occurs in the editor, so that ticket is not a concern.
If the first texture exists, you can get its format then create an image with that same format.
The only challenging part is if its a compressed format. You could have a list of compressed formats, and if it's in that list, you can use the compression function. It should work fine for 80-90% of texture types.

If they have a rare compressed texture that godot can't create, then they'll get an error and a broken display until they drop in a new texture, which is acceptable since it will happen in a small subset of rare textures people will only use <1% of the time.

@TokisanGames
Copy link
Owner

Updated the PR after a lot more testing. Thanks!

@TokisanGames TokisanGames merged commit ac4d420 into TokisanGames:main Jan 7, 2024
@TokisanGames TokisanGames mentioned this pull request Jan 7, 2024
@lfxu lfxu deleted the formats branch January 22, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants