davidfig / pixi-viewport

A highly configurable viewport/2D camera designed to work with pixi.js
https://davidfig.github.io/pixi-viewport/
MIT License
1.04k stars 174 forks source link

Custom render() #244

Open davidfig opened 4 years ago

davidfig commented 4 years ago

Original message from Shukant:

Here’s how I kind of mixed the two things:

These changes would work as-is; except if users are accessing viewport.children directly, they won’t get the actual contents. (Since we override addChild, etc. to add the children into “viewport.contents.children”). It would kind of be a breaking change.

davidfig commented 4 years ago

I remember the discussion below. What type of perf improvements are you getting with this? If I understand it correctly, you exchange the children’s local matrix recalculations for a single matrix multiplication at the viewport level in the render loop.

It will result in developer confusion/refactoring when dealing with viewport.children. (I’ve used viewport.children directly in some of my projects, but not terribly often.) If the perf improvements are big enough then it would be totally worth it.

davidfig commented 4 years ago

Oooh, now that I think about it, if we derive Viewport from DisplayObject instead of Container we can fix the children issue and have no breakage for the API users. We'd have to duplicate some Container code but it shouldn't be too bad (famous last words).

ShukantPal commented 4 years ago

@davidfig We did get a big boost in FPS (the google-material-design file, which had ~3300 objects, renders at 50 FPS at .12-.15x zoom).


You don't have to derive from DisplayObject. Rather, you can override the methods of Container so they redirect children into viewport.contents.

class Viewport {
  addChild(...children) {
     this.content.addChild(...children);
  }
}
davidfig commented 4 years ago

Cool. Then it's worth doing.

The advantage to overriding the DisplayObject is that you can directly access viewport.children. I don't think there's a way of doing that if you derive from Container. This fixes backwards compatibility. The downside is that you'd have to maintain the children part of the code.

Thinking about it, I'm good either way. I'll leave it to you to figure out which one is better/easier to develop and maintain.

Thanks for your help on this!

ShukantPal commented 4 years ago

@davidfig Unfortunately, I forgot to tell you. This will have to wait on - https://github.com/pixijs/pixi.js/pull/6683.

That PR fixes a major bug with the projection/render-texture system. pixi-viewport will also have to use the next patch version of pixi.js

(I actually created a "patches" file for pixi.js in our renderer; when the next version of pixi.js would come out, I would've removed those patches. But that doesn't make sense for a library)

davidfig commented 4 years ago

No problem. We'll hold off on merging this to master until pixi releases its new version, and then add a minimum pixi version once it's released.

ShukantPal commented 4 years ago

I just realized that some more patches need to be made in pixi.js.

Specifically, the filter-system doesn't respect the source/destination frames when rebinding the last/original render-texture: https://github.com/pixijs/pixi.js/issues/6461

Another one is the interaction system needs to support transformations for mouse coordinates.

I'm working on creating these patches.