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

Eye rotation doesn't work with inherited colors #106

Closed
wants to merge 23 commits into from

Conversation

mustanggb
Copy link
Contributor

Fixes #105.

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2022

Codecov Report

Merging #106 (5549ad5) into master (5c2d190) will increase coverage by 2.07%.
The diff coverage is 80.26%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #106      +/-   ##
============================================
+ Coverage     63.42%   65.49%   +2.07%     
- Complexity      928      939      +11     
============================================
  Files            47       48       +1     
  Lines          2783     2855      +72     
============================================
+ Hits           1765     1870     +105     
+ Misses         1018      985      -33     
Impacted Files Coverage Δ
src/Renderer/Path/Curve.php 0.00% <0.00%> (ø)
src/Renderer/ImageRenderer.php 91.52% <50.00%> (-3.03%) ⬇️
src/Renderer/Eye/GridEye.php 100.00% <100.00%> (ø)
src/Renderer/Path/Close.php 100.00% <100.00%> (ø)
src/Renderer/Path/EllipticArc.php 34.67% <100.00%> (+34.67%) ⬆️
src/Renderer/Path/Line.php 100.00% <100.00%> (ø)
src/Renderer/Path/Move.php 100.00% <100.00%> (ø)
src/Renderer/Path/Path.php 89.18% <100.00%> (+14.18%) ⬆️
src/Renderer/RendererStyle/Fill.php 82.92% <0.00%> (+4.87%) ⬆️
src/Renderer/Image/ImagickImageBackEnd.php 69.03% <0.00%> (+5.80%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c2d190...5549ad5. Read the comment docs.

@mustanggb
Copy link
Contributor Author

If you want a test is it okay to add my non-symmetrical eye interface to the codebase?

@mustanggb
Copy link
Contributor Author

No idea why the test is failing, is there anyway to see the file generated by the CI?

@mustanggb
Copy link
Contributor Author

Okay, figured it out.

The code was fine and the files were identical to the human eye.

Just slight differences probably due to imagick version differences between my system and the test platform.

So I extracted the files from a workflow artifact and now the tests pass.

@DASPRiD
Copy link
Member

DASPRiD commented Mar 14, 2022

An alternative would be to test with the SVG renderer, since that output is predictable.

@mustanggb
Copy link
Contributor Author

Yup, definitely a safer option, but perhaps better as a follow-up to update all the integration tests.

@mustanggb
Copy link
Contributor Author

@DASPRiD Any feedback, or things you'd like addressed?

@DASPRiD
Copy link
Member

DASPRiD commented Mar 14, 2022

Generally this looks good to me. I'm not sure about the GridEye naming, that's not very descriptive for what it actually is. I cannot think of a better term from the top of my head right now though.

@DASPRiD
Copy link
Member

DASPRiD commented Mar 27, 2022

How about you rename it to PointyEye, I think that might be more descriptive. Apart from that, the PR looks good to me.

@DASPRiD
Copy link
Member

DASPRiD commented Mar 18, 2024

@mustanggb I haven't heard back from you ever. I'm currently preparing a new major release. If I don't hear back from you in the next few days, I'm going to close this PR.

@mustanggb
Copy link
Contributor Author

Heard back about what?

I was just waiting for it to be merged.

@DASPRiD
Copy link
Member

DASPRiD commented Mar 18, 2024

@mustanggb See my comments from March 14 and 27, 2022 :)

@DASPRiD DASPRiD deleted the branch Bacon:master March 19, 2024 00:32
@DASPRiD DASPRiD closed this Mar 19, 2024
@DASPRiD
Copy link
Member

DASPRiD commented Mar 19, 2024

Also, please rebase against the new main branch.

@mustanggb
Copy link
Contributor Author

Just a rename then, I don't mind it.

Rename and rebase at #174.

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.

Eye rotation doesn't work with inherited colors
3 participants