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

More work on splitting up RendererStorage #59984

Merged
merged 3 commits into from
Apr 19, 2022

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Apr 7, 2022

I plan to handle a number of different RendererStorage related tasks here.

Done:

  • Merged Canvas, Texture and Decals into TextureStorage
  • Moved RenderTarget into TextureStorage
  • Move light, lightmap and probe into LightStorage
  • Extract particles and related classes into ParticleStorage

Todo (I'll do these in separate PRs afterall, it just gets hard to manage if I try to do all in one):

  • Extract VoxelGI into VoxelGIStorage or see if we can move this into the VoxelGI implementation classes
  • Extract Fog into FogStorage or see if we can move this into the Fog implementation classes

@BastiaanOlij BastiaanOlij added this to the 4.0 milestone Apr 7, 2022
@BastiaanOlij BastiaanOlij self-assigned this Apr 7, 2022
@BastiaanOlij BastiaanOlij requested review from a team as code owners April 7, 2022 14:05
@BastiaanOlij
Copy link
Contributor Author

@clayjohn I may have really broken GLES3 here, the problem I ran into is that some of the initialization from RendererStorage has moved into the constructors of the different storage classes.

I think we may need to change the way the GLES3 renderer is organized and do the same thing as with RendererRD. i.e. instead of embedding the instances into the rasterizer class, change them to pointers and instantiate them in the correct order and at the correct time while we set things up.

@clayjohn
Copy link
Member

clayjohn commented Apr 7, 2022

That sounds like I good idea. I will take a closer look when I can. I have a feeling #59968 is related.

@BastiaanOlij
Copy link
Contributor Author

I need to start doing more testing on GLES3, it's easy to get something mixed up. Though it compiles for me so wonder what that other issue is about, weird issue

@BastiaanOlij BastiaanOlij force-pushed the more_storage_20220407 branch 4 times, most recently from 5061eb3 to 5dacbd3 Compare April 9, 2022 14:00
@Calinou
Copy link
Member

Calinou commented Apr 10, 2022

Running the GLES3 renderer was likely already broken by earlier pull requests (which landed in 4.0alpha6). See #60106.

@BastiaanOlij
Copy link
Contributor Author

@Calinou to be honest, once we split all this stuff out it probably is more efficient to just do one big push getting GLES3 cleaned up. I noticed that almost all the stuff I'm splitting out isn't even implemented in GLES3, I'm guessing because we've only done the 2D stuff and I've hardly touched that so..

@BastiaanOlij BastiaanOlij force-pushed the more_storage_20220407 branch 3 times, most recently from 910e122 to 317bd5a Compare April 12, 2022 12:27
@BastiaanOlij BastiaanOlij requested review from clayjohn and reduz April 13, 2022 11:02
@BastiaanOlij BastiaanOlij force-pushed the more_storage_20220407 branch from 317bd5a to 0b4fd92 Compare April 17, 2022 03:13
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 mostly fine to me. I am a bit concerned about GLES3, but I am happy to dig into those issues after we merge this PR.

@clayjohn clayjohn merged commit 1d21779 into godotengine:master Apr 19, 2022
@clayjohn
Copy link
Member

Thanks!

@BastiaanOlij BastiaanOlij deleted the more_storage_20220407 branch June 23, 2022 06:36
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