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

don't allow v2.5 of Luxor because of transposing #149

Merged
merged 1 commit into from
Sep 7, 2020

Conversation

Wikunia
Copy link
Member

@Wikunia Wikunia commented Sep 7, 2020

PR Checklist

If you are contributing to Javis.jl, please make sure you are able to check off each item on this list:

  • I think the following points do not apply 😉

  • Did I update CHANGELOG.md with whatever changes/features I added with this PR?

  • Did I update the TODO.md (if applicable)?

  • Did I make sure to only change the part of the file where I introduced a new change/feature?

  • Did I cover all corner cases to be close to 100% test coverage (if applicable)?

  • Did I properly add Javis dependencies to the Project.toml + set an upper bound of the dependency (if applicable)?

  • Did I properly add test dependencies to the test directory (if applicable)?

  • Did I check relevant tutorials that may be affected by changes in this PR?

  • Did I clearly articulate why this PR was made the way it was and how it was made?

How did you address these issues with this PR? What methods did you use?
I've disallowed v2.5 of Luxor as it seems to handle image matrices differently. At one point we probably only want to support v2.5 and above though.

@codecov
Copy link

codecov bot commented Sep 7, 2020

Codecov Report

Merging #149 into v0.2 will increase coverage by 0.32%.
The diff coverage is 99.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##             v0.2     #149      +/-   ##
==========================================
+ Coverage   97.33%   97.66%   +0.32%     
==========================================
  Files           7        7              
  Lines         451      557     +106     
==========================================
+ Hits          439      544     +105     
- Misses         12       13       +1     
Impacted Files Coverage Δ
src/Javis.jl 96.69% <98.92%> (+0.57%) ⬆️
src/luxor_overrides.jl 100.00% <100.00%> (ø)
src/subaction_animations.jl 100.00% <100.00%> (ø)
src/util.jl 100.00% <100.00%> (ø)

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 ed48738...8c703c2. Read the comment docs.

@Wikunia
Copy link
Member Author

Wikunia commented Sep 7, 2020

I'll merge this and we might revert back as @cormullion is going to put the transpose back into it probably tmr.

@Wikunia Wikunia merged commit f0d5dc8 into v0.2 Sep 7, 2020
@Wikunia Wikunia deleted the wik-bugfix-luxor-version branch September 7, 2020 17:53
@cormullion
Copy link
Contributor

I'm taking the transpose out, I thought?

@Wikunia
Copy link
Member Author

Wikunia commented Sep 7, 2020

Wait what? 😂 Does taking it out brings it back to v2.4 behavior or will it be a breaking change and you mention it in the docs?
For us it doesn't really matter. We will handle that inside Javis then.

@cormullion
Copy link
Contributor

Looking at 2.4:

function image_as_matrix()
    if length(CURRENTDRAWING) != 1
        error("no current drawing")
    end
    w = Int(current_surface().width)
    h = Int(current_surface().height)
    imagesurface = CairoImageSurface(fill(ARGB32(1, 1, 1, 0), w, h))
    cr = Cairo.CairoContext(imagesurface)
    Cairo.set_source_surface(cr, current_surface(), 0, 0)
    Cairo.paint(cr)
    data = imagesurface.data
    Cairo.finish(imagesurface)
    Cairo.destroy(imagesurface)
    return reinterpret(ARGB32, permutedims(data, (2, 1)))
end

and 2.5

function image_as_matrix()
    if length(CURRENTDRAWING) != 1
        error("no current drawing")
    end
    w = Int(current_surface().width)
    h = Int(current_surface().height)
    z = zeros(UInt32, w, h)
    # create a new image surface to receive the data from the current drawing
    imagesurface = CairoImageSurface(z, Cairo.FORMAT_ARGB32)
    cr = Cairo.CairoContext(imagesurface)
    Cairo.set_source_surface(cr, current_surface(), 0, 0)
    Cairo.paint(cr)
    data = imagesurface.data
    Cairo.finish(imagesurface)
    Cairo.destroy(imagesurface)
    return reinterpret(ARGB32, permutedims(data, (2, 1)))
end

they kind of look the same... - perhaps you were using a intermediate master?

@Wikunia
Copy link
Member Author

Wikunia commented Sep 7, 2020

So we used v2.4 up to now and GitHub Actions used v2.5 for the last one which broke: https://github.com/Wikunia/Javis.jl/runs/1082538562 with this PR setting the upper bound to only include v2.4 it worked again.
Therefore we expected that you made some breaking change.

@Wikunia
Copy link
Member Author

Wikunia commented Sep 7, 2020

But yeah you make a good point. Can I help in anyway to figure out what happened?

@cormullion
Copy link
Contributor

Yes, please! I don't know where the problem is in Luxor, because the two releases look to be the same, and the transposition was only present in one or two master versions while I was developing the alternative approach.

Luxor 2.4

julia> m = @imagematrix begin
             background("red")
             end 10 1
10×1 reinterpret(ColorTypes.ARGB32, ::Array{UInt32,2}):
 ARGB32(1.0N0f8,0.0N0f8,0.0N0f8,1.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)

Luxor 2.5

julia> m = @imagematrix begin
             background("red")
             end 10 1
10×1 reinterpret(ColorTypes.ARGB32, ::Array{UInt32,2}):
 ARGB32(1.0N0f8,0.0N0f8,0.0N0f8,1.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)
 ARGB32(0.0N0f8,0.0N0f8,0.0N0f8,0.0N0f8)

(Also, both answers are still wrong, so I still have to look into this some time... :( )

@Wikunia
Copy link
Member Author

Wikunia commented Sep 8, 2020

I have the following code:

using Luxor, Images

Drawing(10, 2, :image)
origin()
img = image_as_matrix()
finish()
println(size(img))
img = convert.(RGB, img)
save("luxor_test.png", img)

on Luxor 2.4 it prints (2, 10) and the image is:
luxor_test 2.4

on Luxor 2.5 it prints (10, 2) and the image is:
luxor_test 2.5

@cormullion
Copy link
Contributor

What dark sorcery is this?! :)

@Wikunia
Copy link
Member Author

Wikunia commented Sep 8, 2020

Oh BTW I use a dark theme so the first version actually is white 😂
Also I don't know why the one is white and the other is black. Isn't the default white? Or is it transparent?
Can you reproduce this with the code on your system @cormullion ? I use linux btw

@cormullion
Copy link
Contributor

Yes

2.4:

 2×10 reinterpret(ARGB32, ::Array{ARGB32,2}):

2.5

10×2 reinterpret(ARGB32, ::Array{UInt32,2}):

and yes there's an open issue about why this doesn't work well... :)

@cormullion
Copy link
Contributor

On 2.4:

w = Int(current_surface().width)
h = Int(current_surface().height)
imagesurface = CairoImageSurface(fill(ARGB32(1, 1, 1, 0), w, h))

On 2.5:

w = Int(current_surface().width)
h = Int(current_surface().height)
z = zeros(UInt32, w, h)
imagesurface = CairoImageSurface(z, Cairo.FORMAT_ARGB32)

I thought I was making the initialization slightly faster - but perhaps this is flipping the dimensions somehow?

@Wikunia
Copy link
Member Author

Wikunia commented Sep 8, 2020

Oh yes it does:
v2.5 gives:

Cairo.CairoSurfaceImage{UInt32}(Ptr{Nothing} @0x00000000050b2eb0, 50.0, 100.0, UInt32[0x00000000 0x00000000 … 0x00000000 0x00000000; 0x00000000 0x00000000 … 0x00000000 0x00000000; … ; 0x00000000 0x00000000 … 0x00000000 0x00000000; 0x00000000 0x00000000 … 0x00000000 0x00000000])

and v2.4 gives:

Cairo.CairoSurfaceImage{ARGB32}(Ptr{Nothing} @0x000000000402a540, 100.0, 50.0, ARGB32[ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) … ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8); ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) … ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8); … ; ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) … ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8); ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) … ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8) ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.0N0f8)])

So the 100 and 50 are swapped.

I used

julia> w = 100
100

julia> h = 50
50

@cormullion
Copy link
Contributor

Ha, it won't be the first or last time I get my rows and columns mixed up... :)

I'll put that 2.4 init code back in and go for a 2.5.1.

@Wikunia
Copy link
Member Author

Wikunia commented Sep 8, 2020

It seems odd to me though. fill does the same as zeros, right? So it is something inside Cairo that mixes this up?

@Wikunia
Copy link
Member Author

Wikunia commented Sep 8, 2020

Oh you can do CairoImageSurface(z, Cairo.FORMAT_ARGB32; flipxy=true)

@cormullion
Copy link
Contributor

now why would they put that keyword argument in?

@cormullion
Copy link
Contributor

JuliaGraphics/Cairo.jl#252 perhaps

@cormullion
Copy link
Contributor

no, JuliaGraphics/Cairo.jl#68 looks like the early days of Julia...

@Wikunia
Copy link
Member Author

Wikunia commented Sep 8, 2020

Wait it must be CairoImageSurface(z, Cairo.FORMAT_ARGB32; flipxy=false) then. Sry

@cormullion
Copy link
Contributor

That's a terrible name anyway.... "I'm always getting A and B the wrong way round, so let's add an option called FLIPAB!"

@Wikunia
Copy link
Member Author

Wikunia commented Sep 8, 2020

That's how it works, right? You just transpose and rotate and flip it often enough until it works and call it a feature that you can do that easily 😂

@cormullion
Copy link
Contributor

Hopefully fixed in 2.5.1.

TheCedarPrince added a commit that referenced this pull request Sep 25, 2020
* Wik feature progressmeter (#137)

* Merged the latest master into v0.2
* Using ProgressMeter

* VideoIO to render mp4 without saving temp frames (#138)

* first version of VideoIO with mp4

* fixing links in documentation (#144) (#145)

* fixing links in documentation (#144)

* first version of HowTo (#142)

* Using Animations.jl (#125)

* first version of Animations.jl

* don't allow v2.5 of Luxor because of transposing (#149)

* Wik documentation howto (#148)

* made review changes for HowTo

* docstrings for structs (#150)

* Forgot to push sethue to Animations.jl (#153)

* allow sethue in Animations.jl
* allow 2.5.1 of Luxor
* added extra line for an example of `sethue`

* merge master in v0.2

* Format file for the repository

* Removing old contributing file

* Added info on dependencies and JuliaFormatter

* Revert "merge master in v0.2"

This reverts commit a853a0f.

* Revert files to a853a0f

* Tutorial animations (#157)

* Animation tutorial

* Wik feature morphing v2 (#154)

* ability to morph with fill.

* [WIP] Live Preview (#119)

* Starting proof of concept on live preview

* Reverted Javis.jl to previous formatting

* Began work on proof of concept

* Beginning of the javis image viewer

* Continued work on trying to preview images

* Barebones example of GTK image viewer

* get_javis_frame function

* Added compat for images and updated authors

* Proof of concept javis viewer signal emission

* Temporary fix for Image compat

* Added buttons and todos

* Added functionality for buttons

* Fixed up Project TOML for compat with Images

* Fixed merge conflict logic bug

* Got proof of concept of live viewer working

* Beginning clean up of javis image viewer

* Added ColorTypes and color conversion for frames

* Added ColorTypes dependency

* Cleaned up and adding documentation strings

* Exported get_javis_frame function

* Prepped javis viewer for full integration into Javis

* Added Gtk and GtkReactive deps

* Added liveview kwarg

* Finished prototype of javis viewer

* Updated boundaries for deps

* Clarified docstrings

* Added arrow key navigation through frames

* Updated boundaries per Ole suggestion

* Fixed up formatting for CI

* Made live rendering faster

* Added loop around functionality

* Adjusted boundaries of deps

* Increased speed of image preview display

* Functionalized repeated code snippets

* Bug in javis viewer - identification attempt

* Made javis viewer work completely

* Reversion to a6fa540

* Made short circuit eval of live viewer

* Documented javis viewer thoroughly

* Added docstring

* Qualified usage of LightXML.value func

* Qualified textbox usage

* Formatting... 👀

* test with xvfb on ubuntu

* without --project

* try with project=.

* different idea using DISPLAY

* fix Documentation CI

* Added documentation for get_javis_frame

* Reduced code reuse

* Added some clarifications

* Removed export of get_javis_frame

* Changed returns for testing

* Added GtkReactive to Project

* Tests for javis_viewer

* Formatting again

* Final test case for increment/decrement tests

* Formatting test file

* Final clean up to javis viewer

Co-authored-by: Ole Kröger <[email protected]>

* removed possibility to set the fontsize inside the latex function (#180)

* removed possibility to set the fontsize inside the latex function

* ability to animate text (#162)

* ability to animate text

* Let us follow a path (#163)

* Let us follow a path

* push preview (#182)

* Update Change Log (#183)

* Added info about live viewer and javis frame

* Added info about drawing text

* Improved Action Error Msg (#185)

* Added error msg about defining Video first

* Added unit test and cleaned up code

* How To:  draw_text, follow_path and liveview (#184)

* added draw_text, follow_path and liveview in HowTo

* return 1.0 in interpolation for single frame (#188)

* [Bugfix] scaling to 0 (#191)

* don't ever scale to 0 :D

* [Bugfix] Follow path starting after first frame (#192)

* bugfix for follow_path if not on first frame

* Drafted showcase table for README

* Images for drafted showcase

* Formatted tables for showcase

* Wik examples follow path (#198)

* first version of an examples folder: drawing a car

* convert frame only once (#199)

* no need to convert to ARGB32

* refactoring (#194)

* refactoring

* merge fix renaming

* resolved typos

* Added back headers for table

* Update for Tutorial 5 (#200)

* Remove any files stored in test/images (#195)

* Remove any files stored in test/images

* Removed file check and created it as a unit test

* Unit test for test/images dir to check for additional files

* Added formatting

* Began update of tutorial 5

* Updated gifs

* Fixed gifs

* Added additional explanation on SubAction

* Merged unit with v0.2

* Added example of drawing a path

* Resized gif to 350 x 350

* fixed tutorial links in readme

* Changed compat entries to allow more (#202)

* changed compat entries to allow more

* changelog ready for merge

* Tutorial overhaul (#201)

* explaining Action and Scaling and fixing some others

* removed some "The"

* emoji fixes and some other small things

* Corrected grammar and some light editing

* Fixed some grammar issues

Co-authored-by: Jacob Zelko <[email protected]>

* Added Examples Page (#203)

* Added examples page link

* Added bezier path gif

* Drafted first table for examples

* Rendered examples properly

* Added explanation for example

* Fixed reference

Co-authored-by: Jacob Zelko <[email protected]>
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.

2 participants