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

⚙️[WIP] Adding Effects Manager in runtime. #2901

Merged
merged 28 commits into from
Aug 20, 2021

Conversation

HarsimranVirk
Copy link
Contributor

@HarsimranVirk HarsimranVirk commented Aug 12, 2021

Reopened PR #2736
Context: see issue #2640 and this Trello card.

TODO:

  • Help link (4ian)
  • Add some tests to create a runtime object, add an effect, change its parameters, delete the effect. (4ian)
  • Consider refactoring the layer effect handling with the new effect manager? (4ian)
  • Fix issue with offsets Bypassed for now because of Pixi issues
  • Tested on the webapp (4ian)
  • Tested on mobile (4ian)
  • Allow to skip updates for objects outside the view. We can do this because effects are updated just before the render, at the time we also check if an object should be visible or not on the screen (culling) (4ian)

@HarsimranVirk HarsimranVirk requested a review from 4ian as a code owner August 12, 2021 16:11
@4ian
Copy link
Owner

4ian commented Aug 12, 2021

Thanks for reopening the PR! :)
So I think ideally we would have a few tests, otherwise we're good to go if functionally this works.

  • @Bouh I think you checked this in the past, do you want to give another try? I think you listed some filters that were causing issues? Or maybe this is now addressed by the usage of the proper width/height of objects.
  • @ClementPasteau this is a fairly large PR but addressing interesting stuff in both the C++ codebase, the runtime (game engine) and in the editor too. Feel free to take a look and drop comments if you have some!

@Bouh
Copy link
Collaborator

Bouh commented Aug 13, 2021

Nice job on this long and complex feature!
Here new review from my own.

Effect name Status ❌/✅ Issue
Adjustement Blue parameter has to be 1 by default, not 0.6, also true for layer effect
Advanced bloom Require to add padding manually, otherwise, the effect is cropped. EDIT 4ian: I think this is kind of ok?
ASCII
Beveled edges
Black and White
Blending mode If the value has decimal the layer isn't rendered
Blur (Gaussian) Limited on edges of the game?
Blur (Kawase) Limited on edges of the game?
Brightness
Bulge Pinch
Color Map
Color Replace
CRT
Drak Night
Displacement Idk what really happened, I feel it act strangely Edit 4ian: looks ok?
Dot
Drop shadow Require to add padding manually, otherwise, the effect is cropped. EDIT 4ian: I think this is kind of ok?
Glitch For layer this effect is animated, currently isn't for Object, (see animation frequency parameter) EDIT 4ian: fixed
Glow
Godray This effect works but doesn't like alpha channel (layer and object)
Light Night
Noise
Old Film
Outline Require to add padding manually, otherwise, the effect is cropped EDIT 4ian: I think this is kind of ok?
Pixelate
Radial Blur Center X/Y aren't good and require to add padding manually, otherwise effect is cropped EDIT 4ian: reported on pixi filters
Reflection For layer this effect is animated, currently isn't for Object, (see animation speed parameter) EDIT 4ian: fixed
RGB split Require to add padding manually, otherwise, the effect is cropped. EDIT 4ian: I think this is kind of ok?
Sepia
Tilt shift
Twist Offset X/Y aren't good, radius size seem doubled? EDIT 4ian: reported on pixi filters
Zoom blur Center X/Y aren't good and require to add padding manually, otherwise effect is cropped EDIT 4ian: reported on pixi filters
  • Hint messages for the Object effect and the Layer effect are exactly the same. Edit 4ian: fixed!

  • Still no helper like for behaviors when the list is empty, the help button on this frame opens the object effect page, while the help button on the dialog opens the help page of the object type. Edit 4ian: fixed!
    image

  • An issue appears when I create multiples effects. Effects are spaced out.
    This wasn't present in my previous review. Edit 4ian: fixed!
    image

  • Still no toggleable effects like behaviors: Edit 4ian: not required for this PR, it's a feature request (as the effects list is the same as for layers).
    image

  • Some effects on objects are limited to the current view, like blurring effects. Here the effect is clamped to the view, must be bleeding offscreen through.
    The project for reproducing: limited_effects.zip

2021-08-13.11-44-03.mp4

In meanwhile, Outlines works fine
image

@4ian
Copy link
Owner

4ian commented Aug 17, 2021

I've rebased this on master (but not merged master on this, to avoid extra commits), so please discard your local branch and fetch it again from GitHub if you want to continue working on it ;)
I'll be pushing more changes/improvements :)

@4ian
Copy link
Owner

4ian commented Aug 17, 2021

I've asked on the PixiJS filters repo to see if it's normal for the Twist/Radial Blur/Zoom Blur to have a fixed/strange positioning of the effect according to the object position on the screen: pixijs/filters#304 :)

@4ian 4ian force-pushed the object-effects branch 2 times, most recently from 0a9c7e2 to 20de121 Compare August 18, 2021 15:17
@HarsimranVirk
Copy link
Contributor Author

Add some tests to create a runtime object, add an effect, change its parameters, delete the effect.

@4ian I've tried writing some tests, but it turned out that the effects aren't registered properly while running tests (maybe because the files related to effects are added only after exporting the game?). The tests simply fail, after logging this: LOG: 'Effect "testEffect" has an unknown effect type: "AdvancedBloom". Was it registered properly? Is the effect type correct?'.

@HarsimranVirk HarsimranVirk changed the title ⚙️[WIP] Adding object effects manager in runtime. ⚙️[WIP] Adding Effects Manager in runtime. Aug 18, 2021
@4ian
Copy link
Owner

4ian commented Aug 19, 2021

maybe because the files related to effects are added only after exporting the game

More or less yes! There is no effects because there are no built-in in the game engine (it's only part of an extension) and none are imported in karma.conf.js (which is listing all the files to import for the tests - like what an exported game would do).
I'm adding some tests now with an effect :) This means I will import an effect in karma.conf.js and then use it in the tests :)

@4ian
Copy link
Owner

4ian commented Aug 19, 2021

Tests are there! :)
I've grayed for now the 3 effects that have issues with PixiJS on objects that are not the size of the window (not the best solution, but at least we can progress).
I'm tempted to go ahead like this - even if some effects are not perfect. I'll write the wiki page about object effects and add the help link.

@4ian 4ian force-pushed the object-effects branch 2 times, most recently from 87893f8 to 1122976 Compare August 20, 2021 10:42
* Update method names to make this very obvious
* Also fix wrong parameters
@4ian
Copy link
Owner

4ian commented Aug 20, 2021

Finished by fixing the timing of the update of object effects: it's now done before the rendering (so effects get a chance to update), I've renamed the method updatePreRender to make this extra clear, and it's only called on objects visible on screen (for efficiency, and because these are "effects" so no need to call the update on all objects living in the scene).

Should be good to go now! :)

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

Successfully merging this pull request may close these issues.

3 participants