ruffle icon indicating copy to clipboard operation
ruffle copied to clipboard

core: `ColorTransform` cleanup

Open relrelb opened this issue 2 years ago • 4 comments

Main changes:

  • Merge ColorTransformParams into ColorTransformObject, as it's only relevant for AVM1.
  • Make BitmapData::color_transform work with a generic ColorTransform, which uses fixed-point arithmetic.

Note that Ruffle still calculates color transforms slightly different from Flash. This is probably caused by inaccuracy of the current ColorTransformObject to ColorTransform conversion and/or the ColorTransform application logic itself. Since this requires further research, it'll be fixed in a future PR.

relrelb avatar Sep 09 '22 22:09 relrelb

This basically reverts 57ccb714c606904af49ee9c715bc6182d442be94, which was part of #5021. How come this is now only relevant for AVM1? I thought the plan was to move even more Params objects here, for example, for all the filter effect objects.

torokati44 avatar Sep 10 '22 08:09 torokati44

See: Screenshot_20220910-105435

torokati44 avatar Sep 10 '22 08:09 torokati44

This basically reverts 57ccb71, which was part of #5021. How come this is now only relevant for AVM1?

We already have ruffle_render::color_transform::ColorTransform (and the equivalent swf::ColorTransform) as common structs. These consist of Fixed8 and i16 which, from my understanding, take place in the calculations.

The f64 values are exclusive to AVM1, and this includes the various filter structs as well. So I think we should follow the pattern of AVM1 objects that hold f64 values, and provide a conversion to the common struct.

relrelb avatar Sep 10 '22 09:09 relrelb

Alright then. My original intent with these ...Params structs was also to reduce duplication (not that I actually followed through with it then...), and you usually take good care of that, in whatever way is the most reasonable at any given time. And many things have changed since #5021, so I'm fine with going in a different direction, and yield the [dis]approval to someone else.

EDIT: I really was just trying to remind everyone of the past discussions about this.

torokati44 avatar Sep 10 '22 14:09 torokati44

Going to merge this in order to unblock future PRs that build upon it.

relrelb avatar Sep 23 '22 08:09 relrelb