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

Load line art animation from .gpla file rather than from Blender directly #235

Merged
merged 21 commits into from
Apr 28, 2024

Conversation

DJLevel3
Copy link
Contributor

Still working on polish and performance, but this works! I need to fix the exporter as well (find it here) which is why this is a draft.

Copy link
Owner

@jameshball jameshball left a comment

Choose a reason for hiding this comment

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

Thanks so much for making this, overall it looks great!! Haven't actually run the code yet, and aware you are looking to make it polished and improve performance so will hold off on a full review until later, but just my initial thoughts :)

Source/PluginProcessor.cpp Outdated Show resolved Hide resolved
Source/PluginProcessor.cpp Outdated Show resolved Hide resolved
osci-render.jucer Show resolved Hide resolved
Source/gpla/LineArtParser.cpp Show resolved Hide resolved
@DJLevel3
Copy link
Contributor Author

I've made all the requested changes that I deem feasible for now, I still need to add some other functionality so this will remain a draft for the time being.

@DJLevel3
Copy link
Contributor Author

Fixes #127

@DJLevel3
Copy link
Contributor Author

Fixes #134

@DJLevel3
Copy link
Contributor Author

I'm pretty sure this is release ready! I have made all the changes I intended to make and I have fixed some bugs that I found, so I'm marking this as ready for review!

@DJLevel3 DJLevel3 marked this pull request as ready for review April 17, 2024 15:03
Copy link
Owner

@jameshball jameshball left a comment

Choose a reason for hiding this comment

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

Thanks again so much for all the changes! Sorry that the code review has gotten a bit stricter since the old osci-render days 😅 Keen to keep it easy to add new features :)

Source/LineArtComponent.cpp Outdated Show resolved Hide resolved
Source/PluginProcessor.cpp Outdated Show resolved Hide resolved
Source/PluginProcessor.cpp Outdated Show resolved Hide resolved
Source/PluginProcessor.cpp Outdated Show resolved Hide resolved
Source/PluginProcessor.cpp Outdated Show resolved Hide resolved
Source/PluginProcessor.h Show resolved Hide resolved
Source/PluginProcessor.h Outdated Show resolved Hide resolved
Source/parser/FileParser.cpp Outdated Show resolved Hide resolved
Source/gpla/LineArtParser.cpp Show resolved Hide resolved
@DJLevel3
Copy link
Contributor Author

Hmm, Codacy is failing it for issues that I don't think are actually issues. It seems that it hasn't realized that the "HIDDEN" does actually do something properly when running in Blender, and that "*.gpla" is the file extension.

Copy link
Owner

@jameshball jameshball left a comment

Choose a reason for hiding this comment

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

Thanks again!! I've fixed a few bugs with it - please can you do the following last few changes:

  • Change any variable names/strings/ids etc. for MIDI sync to BPM sync (as discussed on Discord)
  • Update the name of 'framerate' to:
    • 'Frames per second' when BPM sync is disabled
    • 'Frames per beat' when BPM sync is enabled
  • Add tooltips to Animate, BPM Sync, FPS/FPB, and Offset labels/buttons to explain what they do
    • There are plenty of examples of how to do this in the codebase, but reach out if you need help!
  • Do a final test
    • Particularly to make sure that the original Blender implementation is still working fine
    • Also to make sure that there is no weird behaviour around line saving that you spotted before
    • Test both standalone and VST

Copy link
Contributor Author

@DJLevel3 DJLevel3 left a comment

Choose a reason for hiding this comment

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

All looks pretty good except for this one strange extra modulo that doesn't appear to be necessary. I'll compile and test momentarily.

Source/gpla/LineArtParser.cpp Show resolved Hide resolved
@DJLevel3
Copy link
Contributor Author

All works as expected, but I'm going to change one thing that causes unexpected behavior: I'm going to change the fallback BPM to 60 so that ticking the BPM sync box in the standalone doesn't double the speed of the animation.

@DJLevel3
Copy link
Contributor Author

Oops I have to test one more thing, 1 sec

@DJLevel3
Copy link
Contributor Author

The line art's strange behavior is also not occurring, so I think all is good!

Copy link
Owner

@jameshball jameshball left a comment

Choose a reason for hiding this comment

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

All good to go! Just needs a little bit of documentation and we're good to release :)

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