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

Please consider using static const's for SPIRO_CORNER, SPIRO_G4, SPIRO_G2, etc. #28

Closed
ctrlcctrlv opened this issue Sep 18, 2020 · 18 comments

Comments

@ctrlcctrlv
Copy link
Member

Let me begin by saying, this is not really your problem, but rather bindgen's problem. (bindgen does the C interop for Rust, one of the main reasons I think Rust is worth using; without bindgen giving C ABI access it's not that great.)

But, I think it would be good to consider changing it anyway. 🥺😉

Having these as symbols in the library is better than just #define's and makes it easier for bindings to "see" them and work with them. There is also the issue that bindgen really cannot know if 'v' is a signed char or an unsigned char; it is ambiguous. In C this is not a problem. In Rust, it is a headache to convert i8 (c_char) to u8 ('v', etc.).

If you agree @JoesCat, I'm happy to PR this.

Patch: MFEK@bb20451

ctrlcctrlv added a commit to MFEK/libspiro that referenced this issue Sep 18, 2020
See updated README and fontforge#28 (GitHub) for rationale.
@jtanx
Copy link
Contributor

jtanx commented Sep 19, 2020

You can't use variables like those proposed in switch statements fwiw.

@ctrlcctrlv
Copy link
Member Author

Oh, hmm, that's quite a problem when it comes to general applicability of this patch outside bindgen.

The problem with just having a const though, no static, is that the linker complains about multiple definitions.

Perhaps there's no way to make all software happy in this case.

@JoesCat
Copy link
Contributor

JoesCat commented Sep 19, 2020

Well, libspiro was created in 2007, and I've tried to maintain backwards compatibility as much as possible - successfully so far (I think).
Adding the patch is sort of like having the tail wag the dog, because it's going to break compatibility at a source code as well as at an executable level.
If libspiro is to be reusable, we should try to maintain a stable APIs that is usable by FontForge, GIMP, your rust editor, etc, etc, etc...

The best solution to keep everyone happy, is to maintain API stability.
Basically, treat libspiro as a black-box, with API definitions written in stone.

It may be more effort to adapt the rust font editor, but as a result, I really think it is the best direction to go. Rethink the problem. There is a solution.

@JoesCat
Copy link
Contributor

JoesCat commented Sep 20, 2020

it is a headache to convert i8 (c_char) to u8 ('v', etc.).

Possible solution since these are basically the same:
[=91
]=93
a=97
c=99
h=104
o=111
v=118
z=122
{=123
}=125

@JoesCat
Copy link
Contributor

JoesCat commented Sep 20, 2020

I've grabbed some of this patch (Bezier vs bezier), otherwise most of this is going to break a lot of existing code so I'd have to discard everything else.
At this point - Think it's safe to close this issue.

@ctrlcctrlv
Copy link
Member Author

No problem at all. My patch makes bindgen happy, which doesn't matter for you the upstream repo. If it can't be done in a way that's going to satisfy other implementations, it shouldn't be done, and I should just maintain a fork.

@ctrlcctrlv
Copy link
Member Author

(By the way, I think this should be fixed in bindgen and will open a bug there if one doesn't exist.)

@JoesCat
Copy link
Contributor

JoesCat commented Sep 20, 2020

If not a bug - it seems like is a limitation on what you can do.

@ctrlcctrlv
Copy link
Member Author

Well I think most people are just using bindgen as a base and then editing the file it produces, while I'm trying to make bindgen produce perfect code with no edits afterwards, and distributing exactly the file as it comes out of bindgen. So perhaps it's a "little of column A, little of column B." 😉

@ctrlcctrlv
Copy link
Member Author

By the way @JoesCat , I've got a much better API which I'm putting in the spiro-rs crate, it allows you to construct a Spiro like:

use spiro::{Point as SpiroPoint, PointType as SpiroPointType, Spiro};
use spiro::bezier::Bezier;
let points = vec![SpiroPoint { x: 0., y: 0., ty: SpiroPointType::G4, ..Default::default() }, 
                  SpiroPoint { x: 100., y: 100., ty: SpiroPointType::G4, ..Default::default() }];
let s = Spiro::<f32>::from_points(pts); // cf. Spiro::from_points_closed
// Rust will automatically call the impl From<Spiro<F>> for Bezier<F>, which internally calls TaggedSpiroCPsToBezier2
let b: Bezier = s.into();

use spiro::bezier::AsSVG as _; // bring trait method into scope
println!("{}", b.as_svg());

@JoesCat
Copy link
Contributor

JoesCat commented Sep 20, 2020

I started to put together a patch/wedge based off of Raph's 2007 code (relicensed Apache), but decided I need to get other stuff done, however the thought would to to substitute libspiro *.h files with wrapper, something like this:

#define _BEZCTX_INTF_H 1 /* substitute my void version */
void bezctx_moveto(void *bc, double x, double y, int is_open);
#define _BEZCTX_H 1 /* substitute my void version */
struct _bezctx {
    /* Called by spiro to start a contour */
    void (*moveto)(void *bc, double x, double y, int is_open);

...basically - rethinking how to use the same library...

...however, if you have another solution that works....sounds fine.

FYI - if you want others to respect your license, then you should respect other licenses too - base your code off of Raph's relicensed spiro to avoid mixing licenses, or keep the library linked and not merged, otherwise you still need to include the gpl3+ license.

@ctrlcctrlv
Copy link
Member Author

Do you think I've done something wrong here? I wrote quite clearly:

Note: LICENSE file refers only to my build.rs. Refer to libspiro for C code licensing and authorship.

So, only my build script is Apache2. My fork of libspiro is GPL3.

@ctrlcctrlv
Copy link
Member Author

@JoesCat Do you mind helping me find the 2007 Apache 2 code? Not finding it

@ctrlcctrlv
Copy link
Member Author

Nevermind, found it. It's not from 2007 exactly, the re-licensing occurred last year against code from 2007.

https://github.com/raphlinus/spiro

If you're upset with how I worded things, I'll see about using this

@ctrlcctrlv ctrlcctrlv mentioned this issue Sep 21, 2020
@JoesCat
Copy link
Contributor

JoesCat commented Sep 21, 2020

Hi Fredrick,
As you mentioned - you are willing to fork.
When I looked at https://github.com/mfeq/spiro-sys/blob/master/src/lib.rs and https://github.com/mfeq/spiro-sys/blob/master/tests/spiro_to_beziers.rs I realized that it won't stop here, and other parts will bleed into spiro.
This was clearly GPL+ libspiro code and none of this is mentioned in the license file - don't take it personally - I'd say the same for anyone else.

@JoesCat Do you mind helping me find the 2007 Apache 2 code? Not finding it

If you're upset with how I worded things, I'll see about using this

It's the weekend, I needed to go out, do other things, not hover over a keyboard ;-)

Raph relicensed his code, and as your code progresses I assume there will be code blending, mixing-n-matching, therefore, nobody is going to argue about what belongs to what license if your source is traced back to that code. Otherwise, you'd be seeing similar problems to this - which still needs resolving fontforge/fontforge#4077

@skef
Copy link

skef commented Sep 21, 2020

Just for the record I don't think that fontforge/fontforge#4077 was really left unresolved. The primary blocking issue are the remaining files that started out GPL licensed, which are mostly related to the word list stuff. That is, it's a todo issue and not a legal question.

As it happens no one as yet has felt strongly enough about changing back to BSD to work through the replacing but those resources could still be found.

If that's right the FontForge project is approaching the question with some care. If the language about the per-file licenses hadn't been included the change wouldn't be tractable, but it was so it is. And of course any code copied or derived from Raph's work should take as much care, so that (for example--I haven't looked into this and am not making a claim) GPL-licensed code is not "laundered" through a Rust translator into a different license.

@ctrlcctrlv
Copy link
Member Author

Let's slow down and look at what really happened here.

There's no laundering going on. Actually, the first version was not transpiled, it was bound to the C libspiro which was compiled with a C compiler. It's a binding. I mentioned in mfeq/spiro-sys README.md that the C code is not under the same license. The only part of that code that is Apache2 is build.rs, which does the building. Only that file can be used Apache2, theoretically if someone wants a reference for using bindgen.

The second version, which I'll be releasing soon, is pure Rust and is based on raphlinus/spiro, which is already Apache 2. So, what I'm doing is not "laundering". I'm simply not using the fontforge version and going all the way back to @raphlinus' version because (a) it's simpler and works better with C2Rust and (b) its license fits the goals of my project better.

Hope this clarifies things.

@JoesCat
Copy link
Contributor

JoesCat commented Sep 21, 2020

Well - glad to see this went from panic mode -> to a better solution that resolved several issues.

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

4 participants