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

RenderingServer reorganization #44094

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

reduz
Copy link
Member

@reduz reduz commented Dec 4, 2020

This PR reorganizes and renames most of RenderingServer classes in hopes that it's easier to understand them. Following diagram shows what each is:

image

@reduz reduz requested a review from vnen as a code owner December 4, 2020 18:32
@Riteo
Copy link
Contributor

Riteo commented Dec 4, 2020

I don't really know how this worked before, but out of curiosity in my ignorance, why is there no RenderingDeviceGLES3?

@jordo
Copy link
Contributor

jordo commented Dec 4, 2020

I like the rename. "raster" -> "render" seems appropriate.

@JFonS
Copy link
Contributor

JFonS commented Dec 4, 2020

@Riteo RenderingDevice is an abstraction layer of moder graphics APIs (Vulkan/DX12/etc.) so GLES3 doesn't really fit in. Instead, we will have separate renderer implementation that directly interacts with GLES3.

@lawnjelly
Copy link
Member

Just to voice a little dissent 😄 I still have a suspicion these new names may prove to be overly long.

Using the word Render twice in a classname rings alarm bells imo. In many cases the original two word form (such as RasterizerCanvas) seems preferable in terms of readability, even though it is not really a rasterizer. Maybe just switching to RendererCanvas and RendererScene might be enough.

Namespaces, or prefixes if avoiding namespaces - like Rs for RenderingServer and Rd for Rendering Device could be an option if wanting to include extra information. I'm not sure adding the word server to the names adds that much useful info to outweigh the verbosity etc.

@reduz reduz force-pushed the reorganize-rasterizer-scene branch from 00a37bb to e38945a Compare December 4, 2020 19:32
@reduz
Copy link
Member Author

reduz commented Dec 4, 2020

@lawnjelly done, we thought about the same with @JFonS, so changed the names per your suggestion.

@reduz reduz force-pushed the reorganize-rasterizer-scene branch 2 times, most recently from ebeb851 to d9b2609 Compare December 4, 2020 20: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.

6 participants