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.

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

rewrite to rust #4

Merged
merged 52 commits into from
Jun 6, 2024
Merged

rewrite to rust #4

merged 52 commits into from
Jun 6, 2024

Conversation

Doublonmousse
Copy link
Collaborator

Go from a wrapper to a C++ library to a rust native implementation

@Doublonmousse
Copy link
Collaborator Author

Doublonmousse commented May 18, 2024 •
edited
Loading

There's one test (corresponding to WobbleSmoothed in the original ink_stroke_modeler repo that fails). Haven't figured out yet what's happening there

@Doublonmousse
Copy link
Collaborator Author

Ok, it finally passes tests. Not all tests are reimplemented though. Notable things missings are written in the todo.md file

@Doublonmousse Doublonmousse marked this pull request as ready for review May 18, 2024 18:17
@flxzt
Copy link
Owner

flxzt commented May 24, 2024

It would be nice to have the steps for regenerating the docs documented or even put into a justfile / script. Ideally this would then be added to the docs CI so we ensure the hosted github pages cargo docs are always up-to-date. And the generated intermediary html/pdf files should probably be not checked in, only the source typst?

flxzt
flxzt reviewed May 25, 2024
View reviewed changes
Copy link
Owner

@flxzt flxzt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only did a high level review for now with things that I immediately noticed. The modeler itself seems to be well tested and working fine, so I'll only quickly go over it in a second pass

src/impl_ds.rs Outdated Show resolved Hide resolved
src/impl_ds.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/impl_ds.rs Outdated Show resolved Hide resolved
src/impl_ds.rs Outdated Show resolved Hide resolved
src/testing.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
src/state_modeler.rs Outdated Show resolved Hide resolved
src/testing.rs Outdated Show resolved Hide resolved
src/impl_ds.rs Outdated Show resolved Hide resolved
@Doublonmousse
justfile for documentation
ee91412
@Doublonmousse
minor fixes
119dd8b 
- use`tracing::debug`
- typo for decque
- minor docstring changes
- remove the rust test of clamp for floats
@Doublonmousse
make testing and utils private
5553dac
@Doublonmousse
visibility changes (pub(crate) instead of public)
d0c409c 
- visibility changes
- re add back docs for CI to work
@Doublonmousse
additional visibility changes and fmt
b63013c
@Doublonmousse
remove getters for ModelerInput and ModelerResult
af717c3
@flxzt
Copy link
Owner

flxzt commented Jun 2, 2024

From a code structure perspective this looks good to me now. Do you want to complete the rest of the tasks in the todo here or should they be done at a later time?

One other thing left to do: the README needs an update, mentioning that ink-stroke-modeler-rs is a rust reimplementation now.

@Doublonmousse
Copy link
Collaborator Author

I don't know. I think I can do them now relatively quickly.

  • changing settings might be done later (cause that'd be dependent on the document opened and dpi settings) and that might not be quick
  • API changes would be to change everything to f64 and use rust durations for times to have less conversions going on between rnote and the modeler
  • change error types to be strongly typed (at least in the ink stroke modeler and maybe later changing the code in the rnote side)

@Doublonmousse
Copy link
Collaborator Author

I'm finishing up changing the error types for the update calls only, the rest will probably be in a follow-up (for the API change, going from f32 to f64 seems fine although some tests fail, and I need to double check that I haven't done anything weird and come up with new tests if these fails are f32/f64 related)

@Doublonmousse
Copy link
Collaborator Author

Ok, I'll stop there for today

  • I've changed the readme as you asked
  • I've changed the error types for the update calls and changed the relevant code in the rnote part.

I'll do the rest of the things as a follow-up

@flxzt
Copy link
Owner

flxzt commented Jun 5, 2024

Alright, the change to f64 also looks good to me.

I pushed one last improvement for the cpp-bindings and created the archive branch archive-cpp-bindings.

Once the new conflicts are resolved I'll squash-merge riir

@Doublonmousse
Copy link
Collaborator Author

Okay, conflicts are finally resolved

@flxzt flxzt merged commit 124cde7 into flxzt:main Jun 6, 2024
4 checks passed
@flxzt
Copy link
Owner

flxzt commented Jun 6, 2024

Awesome. Thank you so much again for doing this huge amount of work. :)

@flxzt flxzt mentioned this pull request Jun 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flxzt flxzt flxzt left review comments

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
@Doublonmousse @flxzt