BatchedMesh silent errors on : setMatrixAt, setColorAt, setVisibleAt, etc..

[Makio64]

Description

BatchedMesh have many silent errors on : setMatrixAt, setColorAt, setVisibleAt, etc..

		// if the geometry is out of range, not active, or visibility state
		// does not change then return early
		const drawInfo = this._drawInfo;
		if (
			instanceId >= drawInfo.length ||
			drawInfo[ instanceId ].active === false ||
			drawInfo[ instanceId ].visible === value
		) {

			return this;

		}

So if I do setMatrixAt(outOfRangeIndex, matrix) it will do nothing and return this, I feel we should inform the developer via a warn there is an error.

Also sometimes it return null, this, false

I dont know whats the current policy of threejs for this kind of error and how its manage across three, but good error message help for debugging and avoid issue/stackoverflow thread.

Solution

I suggest to add a little warning like : BatchedMesh instanceId[${instanceId}] wasnt update, this can be cause if the instance is not visible/active or wrong index

Also be consistant in the return

Alternatives

• Factorize for more precise error message.
• Remove the test and let it crash ( so the devs see there is an error and can fix the code )

Additional context

No response

[gkjohnson]

I personally prefer a solution other than logging console warnings which can't be detected with code. My opinion is these functions should return a documented value (like null or false) that indicates whether the function succeeded or an error should be thrown. I think adjusting the functions and documentation to use the first solution (returning null etc if not successful) is the least intrusive.

[Makio64]

@gkjohnson I understand your point, it should be detected by code and less intrusive.

I strongly feel devs will not write the bellow example using the current return value as it feels unatural and heavy :

for(let i=0; i<count; i++){
    if(bmesh.setMatrixAt(i, matrix) == null){
        console.warn(`instanceId[${i}] wasnt update, this can be cause if the instance is not visible/active or wrong index `)
    }
}

Usually dev will write :

for(let i=0; i<count; i++){
   bmesh.setMatrixAt(i, matrix) // silent error
}

So maybe, we want to remove the test and let the error happening like when i do bmesh.undefinedFn() //trigger, undefined is not an Function ? It'll trigger the error and then the dev can fix it easily and not let it silently working with error in the code.

It can also be intercepted by try/catch ( but i think its like with the return null/false, its not a real case scenario )

for(let i=0; i<count; i++){
   try{ bmesh.setMatrixAt(i, matrix) } 
   catch(e){
        console.warn(`instanceId[${i}] wasnt update, this can be cause if the instance is not visible/active or wrong index `, e)
    }
}

[gkjohnson]

I don't consider returning "null" to be a silent error because it's possible to handle - you just have to "listen" for it. Though I understand it's a difference in stylistic code patterns. Three.js hasn't typically thrown errors in some of these these cases previously, though I do prefer it. I'm fine with an error in this case since it would mean a user is book keeping an id they shouldn't be.

If you'd like to make a PR handling these cases I think we can get it merged.