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

Fl_Cairo_Window docs should describe the default coordinate system #358

Closed
erco77 opened this issue Dec 31, 2021 · 14 comments
Closed

Fl_Cairo_Window docs should describe the default coordinate system #358

erco77 opened this issue Dec 31, 2021 · 14 comments
Assignees

Comments

@erco77
Copy link
Contributor

erco77 commented Dec 31, 2021

The Fl_Cairo_Window docs are rather thin. Currently one has to refer to the test programs to understand how to draw anything. (I'm trying to figure this out now)

I'd suggest:

  • Docs describing the coordinate system of the window for cairo drawing, e.g. cairo_move_to(cr,x,y)
  • Short code example showing e.g. "How to draw an X" to the four corners of the window

If I get my test program working, I can write the docs and solve this issue, but currently having trouble just drawing an X.

@erco77 erco77 self-assigned this Dec 31, 2021
@erco77
Copy link
Contributor Author

erco77 commented Dec 31, 2021

Got my example working so assigning myself.
Ill add some docs and other devs more familiar with cairo can check if its right.

Seems the default coordinate space for Fl_Cairo_Window is FLTK's, and one has to watch it with any cairo example code that assumes normalized space (0,0,1,1), which I guess is cairo's default.

And the coord space affects functions like line width, so using cairo examples can end up drawing nothing because the line width is too small for FLTK coord space.

Ill add some docs to include a working example for both FLTK coord space and normalized space, so folks approaching Fl_Cairo_Window from cairo example code can get things to work too.

Should also add a comment the cairo test code about what the cairo_scale(cr,w(),h()) function's purpose, which is to enable cairo's normalized coord space.

@erco77
Copy link
Contributor Author

erco77 commented Jan 1, 2022

Attaching a patch suggestion to add a new examples/cairo-draw-x.cxx file and Makefile mods. The patch adds new CAIRO variables to the examples/Makefile.FLTK, and uses them in the examples/Makefile.

Will follow up with a CMakeLists.txt patch once I get some time to test it.

@Albrecht-S
Copy link
Member

Albrecht-S commented Jan 16, 2022

Will follow up with a CMakeLists.txt patch once I get some time to test it.

@erco77 I took a stab at this (because it's CMake related) and it works well both with and without Cairo enabled. Here's my patch:
issue-358_cmake.diff.txt

I tried your patch (cd examples; make) and noticed that it works with a Cairo-enabled lib but fails if FLTK is not built with Cairo. AFAICT the issue is in fltk-config --use-cairo ... which doesn't work correctly if Cairo is disabled (should it?).

Unfortunately I don't know of a simple way to "know" (in the Makefile) if FLTK has been configured with or w/o Cairo and fltk-config does a very questionable check (does lib/libfltk_cairo.a exist?) which (a) is error prone and (b) will not work in the future because I'm going to remove libfltk_cairo completely.

I believe the better way to go in examples/Makefile would be to use the variables defined in ../makeinclude which is included in examples/Makefile anyway but that's something I'd like to leave to you.

That said, feel free to use my CMake patch...

@erco77
Copy link
Contributor Author

erco77 commented Jan 16, 2022

I suppose if you're going to be removing the cairo lib, then perhaps it'll be a non-issue, as the link would then succeed because the code checks for FLTK_HAVE_CAIRO and shows a dialog.

To fix fltk-config we could probably replace the existing bad lib check with something like egrep -q '^#define FLTK_HAVE_CAIRO 1' FL/fl_config.h. Or worst case, do a similar check in the Makefile. I'll try following up with a v2 patch.

@Albrecht-S
Copy link
Member

I suppose if you're going to be removing the cairo lib, then perhaps it'll be a non-issue ...

Regarding libfltk_cairo: yes. But you'd still need to link to libcairo or not, depending on ... ?

I think the relevant information should be in makeinclude. This defines LINKFLTKCAIRO (w/o embedded underscore) whereas your Makefile defines LINKFLTK_CAIRO (with underscore). Something similar is available for the Cairo compiler flags.

The CMake patch I wrote checks whether FLTK_HAVE_CAIRO is defined (in CMake!) and links in libcairo if true. The if (TARGET ...) check is only to determine if it's the "old style" build (today, with libfltk_cairo) or the future build (w/o libfltk_cairo). When the transition is done this check can be removed.

@Albrecht-S
Copy link
Member

To fix fltk-config we could probably replace the existing bad lib check with ...

This could probably be done better by configure and CMake setting appropriate variables in the fltk-config script which is generated from fltk-config.in by configure and CMake. We could likely add ready-to-use variables rather than guessing.

But I need to look deeper into it (probably tomorrow). This is something I need to do anyway for better CMake integration (some parts that are supported by configure are still missing in the CMake generation of fltk-config).

@erco77
Copy link
Contributor Author

erco77 commented Jan 16, 2022

Attaching a v1 patch fltk-config-v1.patch.txt which seems to solve the issue with fltk-config cleanly. The fact this checks fl_config.h is IMHO pretty solid, as that is what the source code looks to as well.

But there's more than one way to cook an egg, so it can be changed later when you have some time.

@erco77
Copy link
Contributor Author

erco77 commented Jan 16, 2022

Ah, just seeing your msg about using configure/cmake to build fltk-config more appropriately. Sounds fine, I'll leave that to you then.

Mind if I apply my mods along with the above fltk-config patch just to close this issue, and you can revert/modify later with your intended changes to configure/cmake? I'd rather not touch configure myself, I still don't understand it at all -- never did.

@Albrecht-S
Copy link
Member

The only problem I see is if egrep is not available. I'm always wondering what systems we support with fltk-config. IIRC we need a POSIX shell, but what exactly does this mean WRT egrep?

Mind if I apply my mods along with the above fltk-config patch just to close this issue ...

If you think egrep is OK, go ahead. I'd also like to close the issue.

@erco77
Copy link
Contributor Author

erco77 commented Jan 16, 2022

I can use grep instead. Using egrep is an old habit for its support of better regex, and to avoid a probably long outdated command line flag variation across platforms for, IIRC, the -q flag. Might have been one of the really old versions of OSX, or Solbourne (Sun clone) machines I used back in the day.. can't remember.

erco77 added a commit that referenced this issue Jan 16, 2022
    Since this is the first cairo example in the examples directory,
    it necessarily involved changes to the Makefile and to fltk-config
    to properly handle the absence/existance of the cairo libs.

    TBD: Add docs to the cario widget describing coordinate system
    and how it differs from the default cairo normalized coordinate system.
@erco77
Copy link
Contributor Author

erco77 commented Jan 16, 2022

Working on adding your cmake patch -- need to test it first.
Also, separately, want to add some doxygen docs to Fl_Cairo_Window.

@erco77
Copy link
Contributor Author

erco77 commented Jan 16, 2022

OK, committed your cmake patch as 313212b.

erco77 added a commit that referenced this issue Jan 17, 2022
    Elaborated on Fl_Cairo_Window's use of FLTK style coordinates,
    and how this differs from cairo's default native normalized
    coordinate system, and shows how to switch from one to the other.

    Also, small comment fix to the cairo example regarding the "X" color.
@erco77
Copy link
Contributor Author

erco77 commented Jan 17, 2022

Added doxygen docs elaborating on the coord system and a small code example. The following screenshot shows the added text:
screenshot

Hoping that closes this out. Albrecht, feel free to reopen if you think there's anything that needs more.

@erco77 erco77 closed this as completed Jan 17, 2022
Albrecht-S pushed a commit that referenced this issue Jan 17, 2022
Sorry for the noise, this statement was included in my proposed patch.
@Albrecht-S
Copy link
Member

Everything's fine from my POV. I just removed a CMake test statement (cd611e3) which was a leftover from my tests in the CMake patch posted above.

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

No branches or pull requests

2 participants