Fixed incorrect Object.isOnScreen functionality. #4763
Conversation
|
We do not want to use calculate true during rendering, that will add unnecessary calculations. |
|
The travis build... it is running same tests as As for the calculations during rendering, you're absolutely right, but these coords calculations, they are everywhere, can't we do something about these, performance wise? I mean, something on lines of when canvas/object's properties that affect the coords are changed, calculate the coords. Probably by having setter's to object properties might help. But this is all another different issue I think. Going back to thread, I really thought that this isOnScreen not calculating coords is a big problem. It is returning wrong results because of this reason. I think it should calculate and performance issue should be fixed in a different manner, immediately. |
|
So Fabricjs offers an api and some built in interactions. The built in interactions, are used by user of a product, and are probably outside dev complete control, and in that case fabricjs takes care of updating the coordinates. For all other situations, to move an object, you have to write some code, in that code you will call setCoords. But should not be a default behaviours. If you can make setCoords faster, even by just removing some add and multiply, please do it. All the usage of trigonometric functions like sin cos atan and atan2, have been minimized recently because often they give back weird float results. Also isOnScreen needs just the 4 main corners, while setCoords i think updates everything. |
src/shapes/text.class.js
Outdated
| @@ -1130,7 +1130,7 @@ | |||
| if (!this.visible) { | |||
| return; | |||
| } | |||
| if (this.canvas && this.canvas.skipOffscreen && !this.group && !this.isOnScreen()) { | |||
| if (this.canvas && this.canvas.skipOffscreen && !this.group && !this.isOnScreen(true)) { | |||
There was a problem hiding this comment.
this should not change. can you provide any insight for the reason behind this change?
The same apply to object.js
test/unit/object_geometry.js
Outdated
| @@ -324,6 +324,7 @@ | |||
| cObj.setCoords(); | |||
| assert.ok(cObj.isOnScreen(), 'object is onScreen'); | |||
| cObj.top = 1000; | |||
| assert.ok(!cObj.isOnScreen(true), 'object is not onScreen with top 1000 with calculate true and no setCoords call'); | |||
There was a problem hiding this comment.
can we add over this line one that says:
assert.ok(cObj.isOnScreen(), 'object is still wrongly on screen since setCoords is not called and calculate is not set, even when top is already at 1000');
|
I added the extra line so i can merge it, i wrote that comment but probably forgot to submit review |
* Added missing calculate parameter in Object.isOnScreen. * Update object_geometry.js
I have not found any usages of Object.isOnScreen being called with calculate = false. I thought about removing it but, was not sure. Hence, left it as it is, added a couple more tests, removed setCoords calls that were kind of bypassing the functionality being tested.