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

Fix GLES3 changing 2D shadow atlas size is broken #80151

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

WhalesState
Copy link
Contributor

@WhalesState WhalesState commented Aug 2, 2023

Fixes #80148
Fixes #80186
Fixes #80396

@WhalesState WhalesState requested a review from a team as a code owner August 2, 2023 06:12
@WhalesState
Copy link
Contributor Author

it's already clamping the texture size to the max_texture_size allowed so this is just a friendly warning when you set a larger size

@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 2, 2023

there are still 3 issues,
1- you are allowed to set a negative size in project settings which will spam errors to the compiler.
2- we need to apply a smooth texture filter to the texture if it has a small size
3- the editor is not updating the changes on running the scene and it doesn't respect the size in settings

@lawnjelly
Copy link
Member

it's already clamping the texture size to the max_texture_size allowed so this is just a friendly warning when you set a larger size

Ah, I missed this I assumed it was doing a reject of the command. Clamping makes sense. However it does look like the check for no change is made above this, so could be moved below the clamping, maybe we could fix this at same time.

@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 2, 2023

you are right, it's the same in vulkan too, let me edit both

Edit: there's no clamping in vulkan

image

@WhalesState WhalesState force-pushed the Dev1 branch 2 times, most recently from 62c32d3 to 5d8734f Compare August 2, 2023 06:53
@WhalesState
Copy link
Contributor Author

please don't approve since this is not finished, i will try to track why it doesn't respect the new size on running the scene.

@lawnjelly
Copy link
Member

lawnjelly commented Aug 2, 2023

please don't approve since this is not finished, i will try to track why it doesn't respect the new size on running the scene.

If you are still working on it, just convert to draft, there is a (hard to see 😀 ) button on github on the top right of the page in the reviewers section. Then reviewers know to hold off, and it also won't be merged by accident. Then click ready for review to move out of draft when finished.

@WhalesState WhalesState marked this pull request as draft August 2, 2023 08:03
@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 2, 2023

this was helpful, now we need to put this line somewhere when the game starts to load settings from project settings.

RS::get_singleton()->canvas_set_shadow_texture_size(GLOBAL_GET("rendering/2d/shadow_atlas/size"));

it already exists in the editor _load_from_settings() so where to add it ? in the ready() of the game in main.cpp

@akien-mga
Copy link
Member

Is this meant to fix #80148? If so, please mention it in the PR description, ideally with "Fixes #80148" which is interpreted by GitHub so it would close the issue if/when the PR merged.

@akien-mga akien-mga added this to the 4.2 milestone Aug 2, 2023
@akien-mga akien-mga changed the title fix GLES3 changing 2d shadow atlas size is broken Fix GLES3 changing 2D shadow atlas size is broken Aug 2, 2023
@WhalesState WhalesState closed this Aug 2, 2023
@WhalesState WhalesState reopened this Aug 2, 2023
@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 2, 2023

@lawnjelly have a look on this

it seems like the shadow_atlas gets resized on gles3 then the shadow polygons are drawn over the small texture and rendered in high resolution on screen

this supposed to be a shadow atlas of size [32]
image

this is [128]
image

and this is [2048]
image

@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 2, 2023

while in vulkan it looks like this

while both seems to have issues in resize but the GLES3 is much cleaner in drawing polygons over a small texture

[32]
image

[128]
image

[2048]
image

zooming in [2048]
image

using Camera2D to show actual pixel size on screen
image

@WhalesState WhalesState marked this pull request as ready for review August 2, 2023 16:06
@WhalesState WhalesState requested a review from a team as a code owner August 2, 2023 16:06
@WhalesState WhalesState marked this pull request as draft August 2, 2023 20:35
@WhalesState WhalesState marked this pull request as ready for review August 2, 2023 21:21
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Great work! I just left one suggestion, after fixing that one thing this should be ready to merge

drivers/gles3/rasterizer_canvas_gles3.cpp Outdated Show resolved Hide resolved
@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 16, 2023

@clayjohn Thank you, please feel free to review and edit the code since I have no experience with opengl, I also have found another issue on this file.

the default polygon color in opengl is black, and here it doesn't update the polygon color if it's white, means drawing with white color will not update the polygon color in opengl but any other color will do, this issue only appears when building without Vulkan and trying to draw any polygon with white color, like a line or a rect inside the draw function will appear black. can i fix this one too ?

it happens like, the first draw call is black when the color is white, the second draw call will be red when the color is red and will fix the first call because it will update the polygon color after this, also another draw call in white color may fix it as i remember since the color array will have more than one color and will not use those fixed lines as i understand, and also will fix the polygon color, so using only one draw_rect with white color will appear black if you didn't draw any other polygons in the same frame. is there's a better way to set the default polygon color to white when the gles3 renderer starts ?

like running this line once when it starts glVertexAttrib4f(RS::ARRAY_COLOR, 1.0, 1.0, 1.0, 1.0);
another option is to give the polygon buffer color a default white value in the .h file, then using glVertexAttrib4f(RS::ARRAY_COLOR, pb->color.r, pb->color.g, pb->color.b, pb->color.a)

image

@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 16, 2023

edited the wrong line. lol!

sorry, got confused. it's the correct line.

thought it's inside free_polygon!
image

also the change in the .h file is not needed, i will test without it to see if the issue is fixed then will remove the default polygon buffer's color since it will be updated in every draw call.

Fixed, will remove the changes from the .h file when the checks are finished.
image

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good now! Thanks for working on this.

As a note for the future, we try really hard to keep separate changes in their own PRs. So, in the future, please open a new PR when you want to fix a separate bug. In this case I won't make you open a new PR because it's literally a two line change. But even for small changes like this it really is better to open a separate PR.

@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 16, 2023

Got it, Thank you ^^
Should I keep the changes in the header file or remove them after the checks? since they make no difference to those fixes.

Anyway, it's ok to merge at this point since it can tell that we are using white as the default polygon color.

@clayjohn
Copy link
Member

Should I keep the changes in the header file or remove them after the checks? since they make no difference to those fixes.

You can keep them. I prefer being explicit about initialization anyway.

@akien-mga akien-mga merged commit 5541567 into godotengine:master Aug 17, 2023
@akien-mga
Copy link
Member

Thanks!

@WhalesState WhalesState deleted the Dev1 branch January 22, 2024 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants