Skip to content

Added effect transformation for images #3028

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 8, 2018
Merged

Added effect transformation for images #3028

merged 1 commit into from
Feb 8, 2018

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Nov 11, 2016 •
edited
Loading

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
License MIT

What's in this PR?

Add the imagine effects as sulu image transformers.

Example Usage

Old format:

    <format>
        <name>400x400-effect</name>
        <commands>
            <command>
                <action>scale</action>
                <parameters>
                    <parameter name="x">400</parameter>
                    <parameter name="y">400</parameter>
                </parameters>
            </command>
            <command>
                <action>blur</action>
                <parameters>
                    <parameter name="sigma">6</parameter>
                </parameters>
            </command>
        </commands>
    </format>

New format:

    <format key="400x400-effect">
        <meta>
            <title lang="en">400x400</title>
        </meta>
        <scale x="400" y="400"/>
         
        <transformations>
            <transformation>
                <effect>blur</effect>
                <parameters>
                    <parameter name="sigma">6</parameter>
                </parameters>
            </transformation>
        </transformations>
    </format>

TODO

  • blur own transformer
  • gamma effect transformer
  • grayscale own transformer
  • negative own transformer
  • sharpen own transformer

@alexander-schranz alexander-schranz force-pushed the feature/effect-transformation branch 3 times, most recently from 2b58c0b to f445e84 Compare November 11, 2016 14:42
@alexander-schranz alexander-schranz mentioned this pull request Nov 11, 2016
Open
@danrot
Copy link
Contributor

danrot commented Nov 15, 2016

If you look at the new format I'd say it is strange that there is a line <effect>effect</effect>. Wouldn't you also prefer to have own actions for these effects?

Would look like this:

    <format key="400x400-effect">
        <meta>
            <title lang="en">400x400</title>
        </meta>
        <scale x="400" y="400"/>

        <transformations>
            <transformation>
                <effect>blur</effect>
                <parameters>
                    <parameter name="???">6</parameter>
                </parameters>
            </transformation>
            <transformation>
                <effect>greyscale</effect>
            </transformation>
            <transformation>
                <effect>negative</effect>
            </transformation>
            <transformation>
                <effect>sharpen</effect>
            </transformation>
        </transformations>
    </format>

@alexander-schranz
Copy link
Member Author

@danrot ok an own transformer for each effect then? was oriented by image because this function all or part of ->effects()->....

The blur parameter is called sigma in imagine use the same in the xml?

@danrot
Copy link
Contributor

danrot commented Nov 15, 2016

@alexander-schranz Yeah, I would say so. I have also seen that the sharpen effect can take two parameters (they are only prefilled with values in Effects. That would not even be possible with your structure.

@danrot danrot closed this Jun 28, 2017
@danrot danrot reopened this Jun 28, 2017
@alexander-schranz alexander-schranz force-pushed the feature/effect-transformation branch from f445e84 to 9b3809d Compare December 13, 2017 09:36
@alexander-schranz alexander-schranz changed the base branch from develop to master December 13, 2017 09:36
@alexander-schranz alexander-schranz force-pushed the feature/effect-transformation branch 2 times, most recently from f0098b5 to 69ef33c Compare December 13, 2017 09:46
@alexander-schranz alexander-schranz force-pushed the feature/effect-transformation branch 3 times, most recently from bea3b57 to f0dbdad Compare February 4, 2018 22:24
@danrot
Copy link
Contributor

danrot commented Feb 8, 2018

LGTM. @chirimoya Are the xml definitions ok for you? Then we could merge it. Would say it is also ok to merge into master, since it is only adding a few new options.

@danrot
Copy link
Contributor

danrot commented Feb 8, 2018

And @alexander-schranz, you should rebase the branch :-)

@alexander-schranz alexander-schranz force-pushed the feature/effect-transformation branch from f0dbdad to f82bbed Compare February 8, 2018 07:53
@alexander-schranz
Copy link
Member Author

@danrot rebased

@danrot danrot merged commit 4eb6ad4 into sulu:master Feb 8, 2018
@alexander-schranz alexander-schranz deleted the feature/effect-transformation branch February 8, 2018 15:57
@alexander-schranz alexander-schranz mentioned this pull request Jun 21, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@danrot danrot danrot approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexander-schranz @danrot