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

Add filter effects classes #641

Merged
merged 60 commits into from Jan 24, 2020
Merged

Conversation

wieslawsoltes
Copy link
Contributor

@wieslawsoltes wieslawsoltes commented Jan 18, 2020

What does this implement/fix? Explain your changes.

  • Added filter effects classes:

    • feBlend
    • feComponentTransfer
    • feComposite
    • feConvolveMatrix
    • feDiffuseLighting
    • feDisplacementMap
    • feFlood
    • feImage
    • feMorphology
    • feSpecularLighting
    • feTile
    • feTurbulence
  • Added component transfer function classes:

    • feFuncA
    • feFuncB
    • feFuncG
    • feFuncR
  • Added light source classes:

    • feDistantLight
    • fePointLight
    • feSpotLight
  • Set correct default values for SvgFilter properties.

  • Added FilterUnits and PrimitiveUnits properties to SvgFilter class.

  • Added X, Y, Width and Height properties to SvgFilterPrimitive class.

  • Added SvgNumberCollection data type similar to SvgPointCollection.

Any other comments?

I would also like to change type of the StdDeviation property of the SvgGaussianBlur filter effect as it's not following the SVG specification. This is off-course breaking change. I hope at least we can discuss this here as part of this PR ads SvgNumberCollection data type and uses it extensively.

        [SvgAttribute("stdDeviation")]
        public SvgNumberCollection StdDeviation
        {
            get { return GetAttribute("stdDeviation", false, new SvgNumberCollection() { 0f, 0f }); }
            set { Attributes["stdDeviation"] = value; }
        }

https://github.com/vvvv/SVG/blob/4580a4e7874afb26c2d0b0b45a68e6b8ae65640c/Source/Filter%20Effects/feGaussianBlur/SvgGaussianBlur.cs#L221-L238

https://www.w3.org/TR/SVG11/filters.html#feGaussianBlurElement

stdDeviation = "<number-optional-number>"
The standard deviation for the blur operation. If two <number>s are provided, the first number represents a standard deviation value along the x-axis of the coordinate system established by attribute ‘primitiveUnits’ on the ‘filter’ element. The second value represents a standard deviation in Y. If one number is provided, then that value is used for both X and Y.
A negative value is an error (see Error processing). A value of zero disables the effect of the given filter primitive (i.e., the result is the filter input image). If ‘stdDeviation’ is 0 in only one of X or Y, then the effect is that the blur is only applied in the direction that has a non-zero value.
If the attribute is not specified, then the effect is as if a value of 0 were specified.
Animatable: yes.

@tebjan
Copy link
Contributor

tebjan commented Jan 20, 2020

@wieslawsoltes thanks a lot for working on that. in order to merge the PR and to make it more understandable to other prpgrammers, please use a more descriptive commit message in future. something like:

"Added FilterUnits and PrimitiveUnits properties to SvgFilter class"

"Update SvgFilter.cs" doesn't give new info, since the commit already shows that the diff is in that file.

i can highly recommend to use GitExtensions which has text completion when writing a commit message.

@wieslawsoltes
Copy link
Contributor Author

@tebjan ok I will write more descriptive commit messages in the feature.

@wieslawsoltes
Copy link
Contributor Author

@tebjan I have fixed commit messages.

@tebjan
Copy link
Contributor

tebjan commented Jan 20, 2020

@wieslawsoltes thanks a lot, that looks good!

@wieslawsoltes
Copy link
Contributor Author

I have question about Process method for new filter primitive classes. It's out of the scope of this PR to implement filter primitive Process methods, is this acceptable solution to be included in PR?

        public override void Process(ImageBuffer buffer)
        {
            // TODO: Implement feBlend filter Process().
        }

https://github.com/wieslawsoltes/SVG/blob/e7a73d9082a0a03e287db813681e234521eae4ef/Source/Filter%20Effects/feBlend/SvgBlend.cs#L22-L25

Copy link
Contributor

@H1Gdev H1Gdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is still being implemented, so I add a few comments.

Source/Filter Effects/feFlood/SvgFlood.cs Outdated Show resolved Hide resolved
Source/Filter Effects/feBlend/SvgBlend.cs Outdated Show resolved Hide resolved
Source/Filter Effects/feBlend/SvgBlend.cs Outdated Show resolved Hide resolved
@wieslawsoltes wieslawsoltes changed the title WIP: Add filter effects classes Add filter effects classes Jan 22, 2020
@wieslawsoltes
Copy link
Contributor Author

This PR is ready for initial review. I may add some tweaks along the way while I am trying to implement some of the filters in Svg.Skia.

@wieslawsoltes
Copy link
Contributor Author

I would also like to change type of the StdDeviation property of the SvgGaussianBlur filter effect as it's not following the SVG specification. This is off-course breaking change. I hope at least we can discuss this here as part of this PR ads SvgNumberCollection data type and uses it extensively.

        [SvgAttribute("stdDeviation")]
        public SvgNumberCollection StdDeviation
        {
            get { return GetAttribute("stdDeviation", false, new SvgNumberCollection() { 0f, 0f }); }
            set { Attributes["stdDeviation"] = value; }
        }

https://github.com/vvvv/SVG/blob/4580a4e7874afb26c2d0b0b45a68e6b8ae65640c/Source/Filter%20Effects/feGaussianBlur/SvgGaussianBlur.cs#L221-L238

https://www.w3.org/TR/SVG11/filters.html#feGaussianBlurElement

stdDeviation = "<number-optional-number>"
The standard deviation for the blur operation. If two <number>s are provided, the first number represents a standard deviation value along the x-axis of the coordinate system established by attribute ‘primitiveUnits’ on the ‘filter’ element. The second value represents a standard deviation in Y. If one number is provided, then that value is used for both X and Y.
A negative value is an error (see Error processing). A value of zero disables the effect of the given filter primitive (i.e., the result is the filter input image). If ‘stdDeviation’ is 0 in only one of X or Y, then the effect is that the blur is only applied in the direction that has a non-zero value.
If the attribute is not specified, then the effect is as if a value of 0 were specified.
Animatable: yes.

@H1Gdev
Copy link
Contributor

H1Gdev commented Jan 23, 2020

@wieslawsoltes

Please use rebase instead of merge because differences are not displayed correctly.

Source/Filter Effects/feBlend/SvgBlend.cs Outdated Show resolved Hide resolved
Source/Filter Effects/SvgFilterPrimitive.cs Outdated Show resolved Hide resolved
Source/DataTypes/SvgNumberCollection.cs Outdated Show resolved Hide resolved
@mrbean-bremen
Copy link
Member

I would also like to change type of the StdDeviation property of the SvgGaussianBlur filter effect as it's not following the SVG specification.

I also had noticed this, and I think it is ok to change. Given that almost none of the filters is currently implemented correctly (Gaussian blur is there, but only partly fillfulls the spec, and also is very slow), I think this kind of changes will not be a problem.

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be merged as is, even if the implementation of the filters itself is missing - looks like a lot of work done!
I will wait for @H1Gdev to have another look and probably merge tomorrow.

@mrbean-bremen
Copy link
Member

As for the commit comments: I usually use squash merges for PRs with more than one commit (except than the commits are not related), and the commit comments end up as separate lines in the comment. I usually try to sanitize these, but it is good to have already meaningful comments.
Merge policy is one point to agree upon after the move to the new repo...

@wieslawsoltes
Copy link
Contributor Author

I would also like to change type of the StdDeviation property of the SvgGaussianBlur filter effect as it's not following the SVG specification.

I also had noticed this, and I think it is ok to change. Given that almost none of the filters is currently implemented correctly (Gaussian blur is there, but only partly fillfulls the spec, and also is very slow), I think this kind of changes will not be a problem.

I did try to retrofit SvgGaussianBlur filter effect with SvgNumberCollection as StdDeviation property type but failed.

My proposal is to remove current implementation of SvgGaussianBlur Process(and related code) and just fix property type.

@wieslawsoltes
Copy link
Contributor Author

@mrbean-bremen @H1Gdev I have fixed all requested changes and I am done with all work I wanted to do. This is ready for final review and merge (preferable squashing merge?).

@wieslawsoltes
Copy link
Contributor Author

For reference I have partially implement all the filter effects added in this PR in my Svg.Skia library.

https://github.com/wieslawsoltes/Svg.Skia/blob/master/src/Svg.Skia/SvgFilterskExtensions.cs

Source/Filter Effects/SvgFilter.cs Outdated Show resolved Hide resolved
Source/DataTypes/SvgNumberCollection.cs Outdated Show resolved Hide resolved
@mrbean-bremen mrbean-bremen merged commit 3d28821 into svg-net:master Jan 24, 2020
@wieslawsoltes wieslawsoltes deleted the FilterEffects branch January 24, 2020 17:38
@possibleshowdown
Copy link

Hello! Thank you for all of your work on this. To clarify, did this actually implement all of the filters listed, or are they not quite finished? I am using this library to render PNGs from SVGs and have been having trouble with the filters working, particularly feBlend, feComposite, and feColorMatrix. Thanks again!

@wieslawsoltes
Copy link
Contributor Author

To clarify, did this actually implement all of the filters listed, or are they not quite finished?

I did not implement any filters effects.

@possibleshowdown
Copy link

Ah OK, thank you for confirming!

@wieslawsoltes
Copy link
Contributor Author

@possibleshowdown
I am working on alternative rendering backend using SkiaSharp. It does support some of the filters. https://github.com/wieslawsoltes/Svg.Skia

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.

None yet

5 participants