Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WHS.ResizeModule does not play nicely with WHS.PostProcessorModule #389

Open
4 of 19 tasks
antoniocapelo opened this issue Sep 10, 2018 · 4 comments
Open
4 of 19 tasks
Labels

Comments

@antoniocapelo
Copy link

Describe your issue here.

Version:
  • v2.x.x
  • v1.x.x
Issue type:
  • Bug
  • Proposal/Enhancement
  • Question
  • Discussion

Tested on:
Desktop
  • Chrome
  • Chrome Canary
  • Chrome dev-channel
  • Firefox
  • Opera
  • Microsoft IE
  • Microsoft Edge
Android
  • Chrome
  • Firefox
  • Opera
IOS
  • Chrome
  • Firefox
  • Opera

I was having troubles getting the scene to keep its ratio and definition on screen resizes, even though I was using the WHS.ResizeModule with the default options.

I just discovered the issue: I'm using the WHS.PostProcessorModule to add some passes and internally it uses the EffectsComposer which creates a new renderer.

This renderer is a different instance from the one that's being updated on the ResizeModule setSize method.

Not really sure if this is a bug (since I'm guessing there's not a direct fix — the manager cannot return the correct renderer, since the order of the modules is relevant), but at least there could be some note on the WHS.PostProcessorModule Documentation, warning about this scenario.

I'm willing to contribute with a PR if needed.

Cheers

@sasha240100
Copy link
Member

Hi @antoniocapelo !

Any PRs are welcome! :D
PostProcessorModule creates a new renderTarget. Typically there are only inputBuffer and outputBuffer, but some advanced passes may require more which makes this thing unpredictable.

What you can do is to try to update them manually with this pseudo-code:

// WARNING: Not tested

const {composer} = app.use('postprocessing');

app.use('resize').addCallback((width, height) => {
  // Update buffer sizes
  composer.inputBuffer.setSize(width, height);
  composer.outputBuffer.setSize(width, height);
});

@antoniocapelo
Copy link
Author

Hi @sasha240100, I'm willing to help :D Just point me to the better option, would be it be something inside the codebase? A note on the docs?

What I've done was create a ResponsivePostprocessorModule (ignore the name) that does the following:

class ResponsiveEffectComposerModule {
    manager(manager) {
        manager.define('effectComposerResizeHandler');
        const resizeModule = manager.use('resize');

        this.postprocessorModule = manager.use('postprocessor');

        if (!resizeModule) {
            throw new Error('ResizeModule dependency not met. Make sure that ResizeModule is listed as a dependecy of App.');
        }

        if (!this.postprocessorModule) {
            throw new Error('PostProcessorModule dependency not met. Make sure that PostProcessorModule is listed as a dependecy of App.');
        }

        resizeModule.addCallback(this.updateRendererSize.bind(this));
        this.updateRendererSize(window.innerWidth, window.innerHeight);
    }

    updateRendererSize = (windowWidth, windowHeight) => {
        this.postprocessorModule.composer.setSize(windowWidth, windowHeight);
    }
}

I just add this module to the App's module list (in the correct order) and now when I resize the window everything stays sharp.

Probably what you suggested would be needed if my problem persisted?

@sasha240100
Copy link
Member

Looks nice and useful! @antoniocapelo You may even drop !resizeModule checks as they should be already done by ModuleSystem.

We're now working more on https://github.com/WhitestormJS/nextgl , which should become a Three.js replacement for Whitestorm.js v3.x, so most of out time & skills are going to that project. Contact me if you are willing to contribute to that one! :)

@antoniocapelo
Copy link
Author

@sasha240100 check your twitter DM's (:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants