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

Cleanup renderer object creation #640

Closed
pjcozzi opened this issue Apr 17, 2013 · 13 comments
Closed

Cleanup renderer object creation #640

pjcozzi opened this issue Apr 17, 2013 · 13 comments
Assignees
Labels

Comments

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 17, 2013

Some objects use a create function like context.createRenderState; others have an explicit cache like context.getShaderCache().getShaderProgram; and others are just allocated with new like DrawCommand.

Likewise, some things need destroy, others need release, and others do not require anything.

As we add more caching (CC #638), we'll be able to converge. Details TBA.

@ghost ghost assigned pjcozzi Apr 17, 2013
@pjcozzi
Copy link
Contributor Author

pjcozzi commented Jun 28, 2013

In addition material creation is inconsistent with the rest of Cesium.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented May 8, 2014

4ac0a1e made a minor incremental improvement so shader programs are created with context.createShaderProgram and destroyed with destroy. Under the hood they are still cached.

Since the renderer is @private, we can address this further later.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Jun 30, 2015

Temporarily removed the Material texture cache: #2850

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Aug 21, 2015

@lilleyse, let's do a pull request for each bullet:

  • createRenderbuffer / createFramebuffer
  • createVertexArray / createVertexArrayFromGeometry
  • createRenderState
  • createTexture2D / createTexture2DFromFramebuffer / createCubeMap
  • createShaderProgram / replaceShaderProgram
  • createSampler
  • createVertexBuffer / createIndexBuffer

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Aug 24, 2015

@lilleyse here's a few ideas for createRenderState:

  • Remove context.createRenderState, of course.
  • Change the RenderState constructor function to take options, which will have two properties: context and renderState. However, users will not call this directly, instead...
  • Add RenderState.fromCache (which is a static function, not on the prototype), which takes the same options and is basically the implementation of context.createRenderState. Make sure to move the actual cache from Context.js:
var nextRenderStateId = 0;
var renderStateCache = {};
  • I'm not 100% convinced that this actually works with multiple WebGL contexts, but I didn't look closely and I am not aware of any bugs. Think it through, and if there are issues, we can simply move the caches to be per context.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Aug 24, 2015

@lilleyse createCubeMap should be straightforward, do it before 2D textures. Just move the implementation into the CubeMap constructor function, and then update everything. There will only be a few places; cube maps are just used for stars and in the tests.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Aug 24, 2015

@lilleyse createTexture2D / createTexture2DFromFramebuffer will also be pretty straightforward. The only thing of note is to move createTexture2DFromFramebuffer to a static function, Texture.fromFramebuffer.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Aug 24, 2015

@lilleyse for createShaderProgram / replaceShaderProgram, replace them with new ShaderProgram.fromCache and ShaderProgram.replaceCache static functions.

We'll talk in person about createSampler, createVertexBuffer, and createIndexBuffer. Basically, we'll create a new Sampler class, and then we need to think about buffers in context with WebGL 2 uniform buffers and transform feedback.

@lilleyse
Copy link
Contributor

It seems safe to move the cache to RenderState since RenderState doesn't seem to store any GL resources. Are there any examples with multiple contexts that I could test with?

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Aug 24, 2015

Here's a good start (which clearly works so we are probably OK and I'm sure I thought about this at some point): http://analyticalgraphicsinc.github.io/cesium-google-earth-examples/demos/multiple/

Code: https://github.com/AnalyticalGraphicsInc/cesium-google-earth-examples/tree/master/demos/multiple

@lilleyse
Copy link
Contributor

After looking closer, it does initialize its viewport width and height with the context drawing buffer's width and height. This may be a problem in some cases.

Edit: never mind, it initializes to undefined if an explicit viewport isn't given.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Aug 25, 2015

@lilleyse open future pull requests into the renderer-refactor branch.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Aug 25, 2015

@lilleyse for buffers, also check out the code from our book:

I'm not at all advocating for the same approach, but taking a brief look should help identify strengths and weaknesses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants