New batch renderer by surajhanchinal · Pull Request #2 · Vtec234/openage

surajhanchinal · 2018-05-20T15:15:49Z

This includes a lot of modifications to the original code which is entirely unnecessary at this point. We have to revert them to their original state. Most of them are converting from the original unique pointers to shared pointers.

Support for texture arrays, index buffers, batch rendering has been added.

textures and shader can now be created context independently.

used for screenshots and resizing currently

the name will soon be used for wrapping shader variables

Vtec234 · 2018-05-21T21:37:58Z

 void GlShaderProgram::use() const {
 	glUseProgram(*this->handle);

-	for (auto const &pair : this->textures_per_texunits) {


Why remove this?

I made a new system for managing textures which can be found in texturemanager.cpp. So this was not compatible with it. But this can be added back and a workaround can be found. I thought the system for managing various textures and texture units was a little too closed for optimisation.

Ok, i'll look at that - if it's indeed better, there's no need to revert back to the old code.

Vtec234 · 2018-05-21T21:38:45Z

-std::unique_ptr<UniformInput> GlShaderProgram::new_unif_in() {
-	auto in = std::make_unique<GlUniformInput>();
+
+void GlShaderProgram::send_uniform(const GlUniformInput *unif_in) {


This is already done in execute_with.

Yes this is effectively the same program but without the sampler2d stuff as that is being handled by texturemanager.cpp

Vtec234 · 2018-05-21T21:40:37Z

+namespace renderer {
+namespace opengl {
+
+GlIndexBuffer::GlIndexBuffer(size_t size, GLenum usage)


This looks like effectively the same code as for GlBuffer, so we could add an enum for the buffer type there and remove this class.

That would be a better idea

Vtec234 · 2018-05-21T21:41:35Z

+
 	bool has_uniform(const char*) override;

+	int texture_array();


What do these functions do? There is no explanation (add comments please).

The texture_array() function basically sets the uniforms of the textures in the shader. Its a little implementation specific right now, but we can abstract it. the main reason here is that I do not have to pass in texture objects to the shader to bind them. They are bound by their own bind() method.

Vtec234 · 2018-05-21T21:43:29Z

 	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
 	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
-
+	glBindTexture(GL_TEXTURE_2D, 0);


This is unnecessary - we don't care which texture is bound as long as we always remember to bind ones we want to use when we need them. Having them bound while they're not being used does not hurt.

I just thought it was a good practice and does not affect performance.

While I can't find a source for this right now, redundant state changes like this can be bad. A single one in the constructor is not important, but as a general rule it's better to avoid them.

Sure. This does not affect the program in anyway

Vtec234 · 2018-05-21T21:45:06Z

+class GlTextureArray final : public GlSimpleObject {
+public:
+	/// Constructs a texture and fills it with the given data.
+	GlTextureArray(const resources::TextureData&);


Should the parameter not be a vector<TextureData> instead? Since there's multiple of them.

I have since written a better version of this. which includes a different constructor and a better way to send in TextureData with the submit_texture() method. Will commit that soon.

Vtec234 · 2018-05-21T21:45:49Z

+       float v;
+    };
+
+    class VertexRenderer{


What does this class do? Again no explanation 😢

this is just a test class. Was trying out different types of rendering such as static draw and such.

Ok, if it's a test class then presumably we won't need to include it in the final code, so there's no need to spend time commenting it.

Vtec234

Hey, first of all thank you for spending time working on this! It's a lot of changes, so will take me some time. In general, I like the texture array and batching additions, but maybe we can figure out a way to integrate them better with the backend-independent renderer abstraction, and make the default Renderer batching by default, removing the need for a separate BatchRenderer.

Like we discussed, removing RVO2 would make this a bit easier to review in the web interface 😅 Additionally, could you please run this through some code formatter conformant with the SFTTech code standards?

Vtec234 · 2018-05-21T22:38:43Z

 	glBufferData(GL_ARRAY_BUFFER, size, 0, usage);
 }

+GlBuffer::GlBuffer(const float *data, size_t size, GLenum usage)


This constructor seems unnecessary, since we can just cast float* to uint8_t*. The meaning of uint8_t is not necessarily that it has to be usable as unsigned ints, but rather just that it's some number of bytes.

Sure. That constructor again is not being used anyway. Lots of testing code left off. I'll remove that as wwel

Vtec234 · 2018-05-21T22:39:12Z

 	glBindBuffer(target, *this->handle);
 }

+void GlBuffer::unbind(GLenum target) const {


Like for the texture, unbinding is not necessary.

A method for doing it wouldn't hurt☺

For maintainability, it's better to have less than more code, so if something is unused, better to remove it.

Vtec234 · 2018-05-22T15:47:40Z

+in float a_id;
+uniform vec2 mouse_pos = vec2(0.0,0.0);
+
+uniform sampler2D texture_0;


To avoid this horrible duplication, a sampler2DArray would serve well.

TheJJ and others added 30 commits November 12, 2017 13:38

buildsystem: inotify package search name lowercased

ec56851

buildsystem: implemented vulkan cmake module

47023b7

buildsystem: integrated vulkan and opengl detection

fa04522

renderer: modular render window and context creation

6c228e7

renderer: legacy opengl square rendering

d1d59c0

renderer: initial generic texture implementation

6454fc8

renderer: rendering quad with opengl 3.3

418838f

renderer: texture and shader creation abstractions

8e170d7

textures and shader can now be created context independently.

renderer: use the full framebuffer for the test quad

ed60e49

renderer: implemented loading shaders from file

0902c83

renderer: initially integrated into the engine

9509f1b

used for screenshots and resizing currently

renderer: renamed pipeline Program to RawProgram

fdb85f6

the name will soon be used for wrapping shader variables

renderer: moved program api contents to separate files

629a86e

renderer: initial pipeline variable abstraction

c53f4cd

renderer: demo id selection via cython

594544f

renderer: first successful texture draw

3094067

renderer: basic uniform variable abstraction

a16d989

renderer: implemented vertex attribute packing and uploading

b1b62b4

renderer: drawing texture with pipeline abstraction

c903e31

renderer: made error checking context-dependent

6b4eaa7

renderer: only upload gpu buffer when changed.

23f2f3f

renderer: towards render tasks by storing uniform state

0c1ef26

renderer: opengl version checking

7170d60

engine: adjust new upstream changes to renderer branch changes

d48a9de

renderer: resource loading refactor

4f7993b

renderer: begin major refactor

a36d4d5

engine: revert changes

9354698

renderer: first working version

77cb221

renderer: documentation and progress

05b3991

renderer: screenshots

c2dc925

surajhanchinal added 4 commits May 17, 2018 10:42

testing phase

b98f9cc

vertexrenderer

5ded02c

Testing the renderer

be12b51

added texture array support

2b2ad9e

Vtec234 reviewed May 21, 2018

View reviewed changes

removing RVO library and adding TerrainManager class

f957e81

Vtec234 reviewed May 21, 2018

View reviewed changes

Vtec234 reviewed May 22, 2018

View reviewed changes

surajhanchinal added 8 commits June 3, 2018 03:56

adding TerrainManager Class

8083d1a

added more abstraction to TerrainManager

615046e

removed test class

6a83f98

removed .vscode from version control

5f3d773

properly removed .vscode folder

31dd5ff

removed unused perspective.h

69f17a7

removing unused libraries

5c3aa4d

better fps counter

54eb090

Vtec234 force-pushed the new-renderer branch from ca15b1b to fff4b9b Compare June 14, 2018 22:08

surajhanchinal added 2 commits June 15, 2018 12:53

testing mulitple renderables

b2e6b09

some more tests with renderable

0627ecc

Vtec234 force-pushed the new-renderer branch 3 times, most recently from cec2431 to f0f6993 Compare July 22, 2018 17:14

Conversation

surajhanchinal commented May 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vtec234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants