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

Fps and field size in settings #1143

Merged
merged 5 commits into from Dec 20, 2018

Conversation

Projects
None yet
4 participants
@davidlamhauge
Copy link
Contributor

davidlamhauge commented Dec 15, 2018

This is a feature I've missed since day one with Pencil2D.
Three crucial values have been hardcoded into Pencil2D, and the is FPS which is 12, and field size which are 800x600 pixels.
For me who always work with 24 fps and 1280x720, it's been annoying to set these values every time I open a scene in P2D, unless its a saved file.
12 fps and 800x600 are still default values, but now every new file will open with the last values you used.
I hope this PR will be accepted.

@CandyFace
Copy link
Member

CandyFace left a comment

Well done David, this will improve Quality of Life for our users.
There are however a few things I'd like see changed before I can approve it.

@@ -84,7 +84,7 @@ class PlaybackManager : public BaseManager
int mMarkOutFrame = 10;
int mActiveSoundFrame = 0;

int mFps = 12;
int mFps;

This comment has been minimized.

Copy link
@CandyFace

CandyFace Dec 15, 2018

Member

Why remove the default value? this could lead to undefined behaviour in worst case scenario.

@@ -65,7 +67,7 @@ class ObjectData
QTransform mCurrentView;

// playback manager
int mFps = 12;
int mFps;

This comment has been minimized.

Copy link
@CandyFace

CandyFace Dec 15, 2018

Member

same comment, this should have a value to avoid undefined behaviours in case the fps won't be set.

QSettings settings (PENCIL2D, PENCIL2D);
int fieldW = settings.value("FieldW").toInt();
int fieldH = settings.value("FieldH").toInt();
if (fieldW < 2)

This comment has been minimized.

Copy link
@CandyFace

CandyFace Dec 15, 2018

Member

You should check against both values.
eg...

Suggested change
if (fieldW < 2)
if (fieldW < 2 || fieldH < 2)

This comment has been minimized.

Copy link
@davidlamhauge

davidlamhauge Dec 16, 2018

Author Contributor

I have reinstated the deault value for mFps to 12.
In layercamera.h I have made two membervariables, int mFieldW=800 and int mFieldH = 600.
I check against both values in layercamera.cpp - just to be sure.

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Dec 20, 2018

Thank you @davidlamhauge I think generally this PR good.
It could be even better to have these settings in the preference window, but it's already a very useful feature for now.

@chchwy chchwy merged commit 87922a3 into pencil2d:master Dec 20, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@davidlamhauge

This comment has been minimized.

Copy link
Contributor Author

davidlamhauge commented Dec 20, 2018

Thanks @chchwy .
I thought about putting them in the preference window, but I decided not to. They are easily accessible in the timeline, so there is no need to have them in the preferences.
What I do want in the preferences, is a chance to select a kind of start up setting, where I can choose Field size, Fps, Filename, Number of layers, Type of layers etc., for my new animation. This should be optional, and the default should as it is.

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Dec 20, 2018

@davidlamhauge There was a discussion about the latter on this issue here #981 I personally think it would be great to have a preset manager on the preferences for such things and now that you mention it would work wonders to have specific layer types and their specified quantity loaded as well, nice thought! 😄

@chchwy chchwy added this to the 0.6.3 milestone Jan 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.