[WIP] Strange code usage in core code that produce side-effects

[eXponenta]

Thanks for this lib, but there are some problems!

Strange code structure that produce side effects.

Folow: #24

So, i think that this Core API look poor, because produce untraceable sideeffects:

1. Program issues
Direct state read from monkey field:

ogl/src/core/Program.js

Line 149 in 2635658

const programActive = this.gl.renderer.state.currentProgram === this.id;

Direct state push into monkey field:

ogl/src/core/Program.js

Line 154 in 2635658

this.gl.renderer.state.currentProgram = this.id;

Why this is problem:

1. Renderer was readed from gl which already is problem, because GLContext not has renderer field, this is pre-mixed field - poor code.
2. We directly set currentProgram from current id that can be manually changed by some reason. Unstable, can produce side-effects!

Possible solution:

Replace this:

ogl/src/core/Program.js

Lines 148 to 154 in 2635658

	let textureUnit = -1;
	const programActive = this.gl.renderer.state.currentProgram === this.id;
	// Avoid gl call if program already in use
	if (!programActive) {
	this.gl.useProgram(this.program);
	this.gl.renderer.state.currentProgram = this.id;

onto this:

this.gl.renderer.useProgram(this.program);

And move check of current active program inside useProgram.

2. Geometry issues

1. Direct state changes and state reset in constructor:
   

   ogl/src/core/Geometry.js

   Lines 43 to 52 in 2635658

   	this.gl.renderer.bindVertexArray(null);
   	this.gl.renderer.currentGeometry = null;
   	// Alias for state store to avoid redundant calls for global state
   	this.glState = this.gl.renderer.state;
   	// create the buffers
   	for (let key in attributes) {
   	this.addAttribute(key, attributes[key]);
   	}

Why this is problem?
Because:

• 1: you enforced drops a vao binding
• 2: allocate a buffers when it not required. This is bad idea because gl can be passed before that renderer was assign.

Also there are a strong reference to renderer state, that can be invalid for draw because can be resets into new object for some reason, for example for binding to another webgl libs, like pixi.

ogl/src/core/Geometry.js

Line 47 in 2635658

this.glState = this.gl.renderer.state;

Possible solutions:
Move preparation step onto draw method and remove reference.

2. Direct state checking and assignment:
   

   ogl/src/core/Geometry.js

   Lines 170 to 174 in 2635658

   	if (this.gl.renderer.currentGeometry !== `${this.id}_${program.attributeOrder}`) {
   	if (!this.VAOs[program.attributeOrder]) this.createVAO(program);
   	this.gl.renderer.bindVertexArray(this.VAOs[program.attributeOrder]);
   	this.gl.renderer.currentGeometry = `${this.id}_${program.attributeOrder}`;
   	}

Why this bad? There are not garanteed that attributeOrder is valid. Should be check on empty value.
This can be easy demonstrated when program is sub class or was invalid. Draw method should be thrownable and crash when invalid state reached.

3. RenderTarget issues (Critical)

3. Texture issues

1. Directly state check and state update while update
   

   ogl/src/core/Texture.js

   Line 86 in 2635658

   if (needsUpdate || this.glState.textureUnits[textureUnit] !== this.id) {

2. State reference holding, again:

   ogl/src/core/Texture.js

   Line 64 in 2635658

   this.glState = this.gl.renderer.state;

Problem that not all texture can was update. VideoTextures MUST wait readyState of HTMLVideoElement. This means that you MUST bind and activate texture but NOT upload it.

Textures should split activation/bind and uploading process.

Possible solution:
Move texture bind and state check to renderer + isolate update and upload.

[ivanpopelyshev]

In pixi its managed by reset() method that is handled by all subsystems, invalidates the stored state

[eXponenta]

Ohohoh
Too many issues. Will be created separated issues for each and PR.

[eXponenta]

Description was updated follow to current version.