Add pixels - Finished. #400

ghost · 2016-06-04T05:20:02Z

This makes pixels an option for measurement.

Update: Got the ruler working too.
Update: Level creation popup and Default Level size in preferences are working now too.

I'd like to see pixels made the default, but that is up for debate.

ghost · 2016-06-04T20:26:07Z

This is in reference to #127

shun-iwasawa · 2016-06-06T05:59:16Z

Unlike in the case of adding other units, I think adding "pixels" unit needs further modification to be included. Because users who use "pixels" unit will expect OpenToonz acting like other movie-editing softwares such as AfterEffects, in which you don't need to mind DPIs of cameras or levels...

For now, when levels placed in the scene, they are scaled to "real size" according to their DPIs. IMO the levels should be placed on pixel-size basis when the current unit is set to "pixels".
Camera settings in the cleanup settings panel and output settings dialog would be better to be simplified when the unit "pixels" is selected; hiding DPI and Inch input boxes.

ghost · 2016-06-06T06:04:02Z

Those are good points. I will look into it. I can easily hide the DPI and other input boxes, but I'm not sure about the scaling part.

I do think that pixels are an important option for users as most paperless animators probably think in terms of pixels over anything else.

ghost · 2016-06-06T06:26:11Z

What do you think about if pixels is set as an option, disabling all DPI options (and hiding them). I'm pretty sure new levels just inherit the current DPI. Hiding the DPI would make sure that all levels are the same.

ghost · 2016-06-06T06:35:53Z

Or is there a generally accepted DPI that is considered actual size that all levels can be locked to if pixels are selected?

shun-iwasawa · 2016-06-06T06:43:10Z

Some images (such as background) may not be created with "New Level" command, but loaded from the file. So I think the software should take care about the case when the DPI of each level is different.
I'm not sure that the idea of "generally accepted DPI" will be accepted by users.

One idea is that when you load the file the software automatically changes its DPI to some specific value to be displayed in the Viewer one-pixel-per-pixel size. For now, DPI can manually changed in the Level Settings.

ghost · 2016-06-09T05:10:49Z

What do you think of when pixels are selected, changing the DPI label to scale- since that's effectively what DPI does when you are talking about scale. We could leave that option visible and let users change the value according to their needs. This would make it easier to use a multi megapixel image and scale it to a smaller video friendly resolution.

What image formats that OT can import have a built in sense of DPI? Is it only PSD files? How do you want to manage them? Use the set DPI in the file or scale them to some other value?

Are there any other major areas of the UI that I should consider with pixels? I will hide the inches sections of the various popups.

shun-iwasawa · 2016-06-15T03:49:30Z

TIFF images can include dpi information and OpenToonz can take into account. (Actually, in studio Ghibli all background images were handled in TIF format in order to exploit dpi feature.)
PNG format also seems to be dpi-available.

Please see stage.cpp line 69:

const double Stage::inch = 53.33333;

This value is used in the viewer with camstand view mode. Which means that if you put the TIFF image with 53.33333 dpi, the image will be displayed in exactly the same pixel size on screen when the viewer is not zoomed. This value is also applied when you load images without dpi such as JPG.
So, if "pixels" is selected, when you load images with dpi value, it would be better to set the level dpi to 53.33333 instead of their own image dpi automatically (with the function LevelProperties::setDpi()).

Are there any other major areas of the UI that I should consider with pixels?

Well, I think some cleanup features are needed to be checked as they use cleanup camera information.

ghost · 2016-06-15T04:40:48Z

Locking the DPI in at 53.3333 makes sense. However, this should only apply to NEW levels created while pixels is set as the option, correct? If switching to pixels makes all existing levels also 53.3333 dpi, that would mess up a lot of projects, right?

Should there be any option for scaling available? If a user imports a huge image with a dpi of 53.3333, it would overwhelm the screen.

I haven't done any scanning/cleanup in OT. Should any DPI information be kept in the scanning/cleanup? Or should it also go to 53.3333?

ghost · 2016-06-15T06:06:59Z

Nevermind on the scaling question. That's what the edit tool is for.

Right now it looks like the camera and new levels default to 64dpi. Should that be made 53.3333 instead?

shun-iwasawa · 2016-06-15T07:48:10Z

However, this should only apply to NEW levels created while pixels is set as the option, correct?

Yes I think so.

If switching to pixels makes all existing levels also 53.3333 dpi, that would mess up a lot of projects, right?

Yes again. I agree with you that the existing scene should not be modified by changing the unit.
Which means that the scene created in "inch" unit may not be shown correctly (=pixel-based) in "pixels" unit because some level may be set dpi other than 53.3333. But I have no idea to resolve such problem across units for now... Maybe displaying some warnings in the dialog?

Right now it looks like the camera and new levels default to 64dpi. Should that be made 53.3333 instead?

Basically yes. When "pixels" unit is selected, I think you need to use 53.3333 dpi both for new level and for camera. But it is not the problem of the default value of dpi. The default values can be changed by users easily with "Save Default Settings" command. However with "pixels" unit, they should be forced always to be 53.3333 in order to show the correct size in the Viewer, and also dpi boxes should be hidden so that the users do not worry about the strange value of "53.3333".

shun-iwasawa · 2016-06-15T11:29:49Z

For now this branch conflicts to master branch since we applied clang-format on v1.0.3 release.
Sorry to trouble you but could you please apply clang-format (with toonz/sources/.clang-format) to your PR or rebase your commits to the current master?
Thank you for your contributions.

ghost · 2016-06-19T03:31:21Z

Do you think that if pixels are enabled at all that they should be used everywhere or should the camera and other units be separate. I have added a checkbox that automatically sets both to be pixels but remembers the previous values and so if the checkbox is disabled, the units go back to their former values.

Also the way I am building it right now, the default camera and levels are always set at 53.3333 dpi, not only when the pixels is selected. The user can change this of course, but if pixels is selected, it will change it back to 53.3333. This is to keep the camera from changing back and forth if people switch from pixels to another unit. (For example: right now the default dpi for the camera is 64. If pixels is selected, it should change to 53.3333 which would alter any scene someone had been working on. If the camera is at 53.3333 by default no matter what, toggling between pixels and something else won't mess up the scene.)

Just a note of warning when the camera is set to 53.3333 with a default resolution of 1920 x 1080, the size is a strange 35.9955 x 20.2544. That might also seem like a strange value. Any thoughts on this?

Here are screenshots of the new functionality. If users should be able to make each individual part pixels, I can take away the checkbox and the part that disables the dropdowns.

ghost · 2016-06-19T03:47:42Z

Not ready yet.

shun-iwasawa · 2016-06-27T09:21:22Z

I took a quick look on your WIP source.
It's very nice idea and implementation to make the "pixels" unit option overriding both the "Unit" and the "Camera Unit", and force "All imported images will use the same DPI" to be ON.

Regarding the default camera dpi, IMO there is no reason to stick to the current value of 64. As you pointed, it would be more friendly if we change it to Stage::inch.

On that condition above, I'm thinking of changing the value of Stage::inch itself... Because it is too small compared to the actual working dpi, and is difficult to hadle as it contains decimals.
Let's say changing the Stage::inch to 160, then the default camera size will be 12 x 6.75 inches. It's much better than 35.9955 x 20.2544 I think.

One problem is that the Stage::inch value are used in many part of the source, especially it defines the size / thickness of the vector levels. In the following image I loaded the same vector level on the software with different Stage::inch values:

So, one idea is to separate Stage::inch into two constant values; i.e. changing Stage::inch to 160.0 and introduce another constant value (say, Stage::vectorDpi) with the value 53.33333 in order to keep the size of vector levels created in older versions unchanged.
I'm not sure this idea will go well but please let me ask your thought about altering the Stage::inch.

shun-iwasawa · 2016-06-27T09:37:35Z

toonz/sources/toonz/preferencespopup.cpp

@@ -1014,18 +1061,19 @@ PreferencesPopup::PreferencesPopup()
  }
  styleSheetType->addItems(styleSheetList);
  styleSheetType->setCurrentIndex(currentIndex);
-
+  //m_pixelsOnlyCB->setChecked(m_pref->getPixelsOnly());
+  m_pixelsOnlyCB->setChecked(true);


Please insert the following in order to disable the combo box if the "pixel" unit is selected on launch.

if (m_pref->getPixelsOnly()){ m_unitOm->setDisabled(true); m_cameraUnitOm->setDisabled(true); }

shun-iwasawa · 2016-06-27T09:57:11Z

Please add a new pixel-based measure for an "fxLength" unit as follows:
The "fxLength" unit is used in the Fx parameters and synchronized with the Unit in the function Preferences::setUnits().

--------------------- toonz/sources/common/tunit/tunit.cpp ---------------------
index a463fa5..60b01bc 100644
@@ -339,22 +339,27 @@ TMeasureManager::TMeasureManager() {
   m->add(cameraYFld.clone());
   add(m);

+  const double stage_inch = 53.33333; //Stage::inch
+
   TUnit fxLength(L"fxLength"),
-      fxInch(L"in", new TSimpleUnitConverter(1 / 53.33333)),
-      fxCm(L"cm", new TSimpleUnitConverter(2.54 / 53.33333)),
-      fxMm(L"mm", new TSimpleUnitConverter(25.4 / 53.33333)),
-      fxXfld(L"fld", new TSimpleUnitConverter(2 / 53.33333));
+      fxInch(L"in", new TSimpleUnitConverter(1 / stage_inch)),
+      fxCm(L"cm", new TSimpleUnitConverter(2.54 / stage_inch)),
+      fxMm(L"mm", new TSimpleUnitConverter(25.4 / stage_inch)),
+      fxXfld(L"fld", new TSimpleUnitConverter(2 / stage_inch)),
+      fxPx(L"px", new TSimpleUnitConverter(1));
   fxInch.addExtension(L"inch");
   fxInch.addExtension(L"\"");
   fxInch.addExtension(L"''");
   fxInch.setDefaultExtension(L"\"");
   fxXfld.addExtension(L"field");
   fxXfld.addExtension(L"F");
+  fxPx.addExtension(L"pixel");
   m = new TMeasure("fxLength", fxLength.clone());
   m->add(fxInch.clone());
   m->add(fxCm.clone());
   m->add(fxMm.clone());
   m->add(fxXfld.clone());
+  m->add(fxPx.clone());
   add(m);

   m = new TMeasure("angle", degree.clone());

ghost · 2016-06-27T16:17:11Z

Thanks for all the great feedback. I am still learning how OT handles units- I will get those changes pulled in.

I like the idea of changing Stage::inch. I was afraid to touch it because I didn't know if it was some special value since it seemed so specific. That solves another issue I didn't like about changing the camera to 53.3333- at that DPI a project of 1920 x 1080 covered the light table completely. This will fix that.

ghost · 2016-06-30T05:20:20Z

@shun-iwasawa At 160dpi, the light table seems a little big compared to the canvas. What do you think of 120? It makes the image size an even 16 x 9.

ghost · 2016-07-13T03:11:27Z

Fixed the error on TUnit and applied clang formatting

ghost · 2016-07-13T03:12:37Z

@shun-iwasawa What do you think about updating Stage::inch in a different PR to keep this one focused on just pixels?

shun-iwasawa · 2016-07-13T06:15:49Z

toonz/sources/toonz/preferencespopup.cpp

+    TDimensionD camSize;
+    camSize.lx = camRes.lx / 53.33333;
+    camSize.ly = camRes.ly / 53.33333;
+    camera->setSize(camSize);


Here you are changing the size of the current camera with dpi value 53.33333.
I think it would be better to do the same thing for the cleanup camera of current cleanup settings as well.
It is stored in CleanupSettingsModel::instance()->getCurrentParameters()->m_camera .

ghost · 2016-07-13T06:29:40Z

Thanks for catching that. I don't use cleanup, so I didn't think about the cleanup camera.

shun-iwasawa · 2016-07-13T06:29:55Z

@turtleTooth First of all, thank you very much for all the way to implementing this feature, it must be one of the big advances of OpenToonz.

What do you think about updating Stage::inch in a different PR to keep this one focused on just pixels?

I agree with your suggestion to keep updating Stage::inch for the future issue.

My guess is that most people are using OT in a paperless format. If this is the case, I think pixels would make a logical default format. Once I implement the hiding of fields, this would benefit new users by having a simpler UI without measurements that seem irrelevant to them. What are your thoughts?

Well, it makes some sense, but IMO making "pixel" unit to be default should NOT be done for now. Maybe at a later date, in consideration of feedback to be provided from users after releasing this, since this change may affect many aspects of the software.

ghost · 2016-07-13T06:31:11Z

Well, it makes some sense, but IMO making "pixel" unit to be default should NOT be done for now. Maybe at a later date, in consideration of feedback to be provided from users after releasing this, since this change may affect many aspects of the software.

That makes sense. I will change it back.

shun-iwasawa · 2016-07-13T06:38:33Z

For now, if you set the default camera dpi to other than 53.33333, it fails to show the image in the viewer with correct size when you create a new scene with "New Scene" command. The "default camera dpi" is stored in the project settings and retrieved with the function TProjectManager::instance()->initializeScene(scene); at line 1288 in iocommand.cpp.
I'm not sure it is the best way but how about updating the camera size after the project settings is loaded when the "pixel" is selected?

ghost · 2016-07-13T06:39:57Z

@shun-iwasawa Thank you for taking the time to review this PR. I realize there is a lot to it. I reverted the default value and agree that feedback should be taken from the community.

shun-iwasawa · 2016-07-13T06:46:06Z

(continued from my previous comment)
... Or, how about opening some confirmation popup to let users to know that they need to change current project settings with 53.33333 dpi?

ghost · 2016-07-13T06:55:46Z

@shun-iwasawa I'm sorry. I think I'm missing how to change the default camera dpi. If pixels only checkbox is selected, it hides the ability to change the current camera dpi. How do I duplicate this error to fix it?

I don't see a way to change the dpi related to project settings.

shun-iwasawa · 2016-07-13T07:15:29Z

"Save Default Settings" command is for saving all settings in the current scene (including camera settings) to the current project settings. The project settings is used for initial settings of new scenes.
If you have not "Save Default Settings" command in the menu bar, it is in "Menu Commands" > "File" category.
So, to duplicate the error, please do as follows:

Select the current unit other than "pixel"
Open output settings and set the camera dpi to some value other than 53.33333
"Save Default Settings"
Select the current unit to "pixel" (this makes the camera dpi of current scene to 53.33333)
Use "New Scene" command (this will retrieve the default camera dpi from the project settings) and see how the camera box is resized in the viewer

ghost · 2016-07-13T07:20:51Z

I just tried that, and it looks like the settings are actually correct, but needs to call a fit to window. I followed your steps and then selected something other than pixels to see the dpi, and they seemed to be right.

Would calling a fit to window after new scene or on scene load fix it?

By the way, sorry for my confusion. I have never used "Save default settings before".

Nevermind- I was able to see what you were seeing. I'll fix it.

…ther than 53.33333

ghost · 2016-07-13T07:52:01Z

Does that fix it? I am seeing the correct settings even if default settings are saved other than 53.33333.

ghost · 2016-07-13T07:56:36Z

(continued from my previous comment)
... Or, how about open some confirmation popup to let users to know that they need to change current project settings with 53.33333 dpi?

I chose the first option, because I think the second might be too many steps:

Undo pixels only
Change dpi settings
Redo pixels only
And they would have to do that for every new scene until they resaved default settings.

shun-iwasawa · 2016-07-13T08:02:28Z

Here I uploaded a video showing the problem:
https://drive.google.com/open?id=0BxLlAdFKGRXTUkxfV2lQMVpNd3c
Anyway, I will check your update right away. Thanks!

ghost · 2016-07-13T08:05:30Z

I'm so sorry for not seeing it right away. Thank you for taking the time to make the video. You rock.

shun-iwasawa · 2016-07-13T09:03:23Z

It seems that the "default dpi on creating new scene" problem is resolved! Thanks!

shun-iwasawa · 2016-07-13T11:33:51Z

toonz/sources/toonzqt/camerasettingswidget.cpp

@@ -277,7 +282,8 @@ CameraSettingsWidget::CameraSettingsWidget(bool forCleanup)
      gridLay->addWidget(m_yPrev, 0, 4, Qt::AlignCenter);

      gridLay->addWidget(m_inchPrev, 1, 0, Qt::AlignRight | Qt::AlignVCenter);
-      gridLay->addWidget(new QLabel("Inch"), 1, 1,
+      QString units = Preferences::instance()->getCameraUnits();
+      gridLay->addWidget(new QLabel(units), 1, 1,


It's a tiny UI problem so I think you can adjust after releasing this PR..

You set the current unit to the label in camera settings instead of "Inch". I think it is very nice modification itself.
But since the label is set once in the constructor, it remains unchanged if you change the camera unit.
Updating the label according to the camera unit or just setting more general name (such as "Size") would be OK.

shun-iwasawa · 2016-07-13T11:34:19Z

Jenkins

ghost · 2016-07-14T03:28:32Z

I like your solution. It is better to update to show the actual measure being used. I will submit it after this is merged.

shun-iwasawa · 2016-07-14T03:40:22Z

OK, It's time to merge and hear the voices from community. LGTM
@turtleTooth Thanks again for your continuous contribution!

ghost · 2016-07-14T03:41:38Z

@shun-iwasawa Yea! Thank you so much. Your help has been invaluable on this.

Jeremy Bullock added 3 commits June 3, 2016 23:06

Adds pixels as an option to the preferences.

96eb784

Ruler tool now working too

99ca93c

Level settings now support pixels

d065744

skitaoka added the UI label Jun 16, 2016

Updated for 1.0.3

29a430c

starter interface options

7c096e2

ghost changed the title ~~Adds pixels as an option to the preferences.~~ Adds pixels as an option to the preferences. WIP Jun 21, 2016

shun-iwasawa reviewed Jun 27, 2016
View reviewed changes

Jeremy Bullock added 3 commits June 30, 2016 20:57

Merge branch 'master' into pixels

5e3b5e8

added shun-iwasawa's suggestions

b99ed3b

set stage::inch to 120

20a966a

Jeremy Bullock added 2 commits July 12, 2016 14:52

Fixed TUnit error

b6ada42

Clang formatting applied

59dbbf4

shun-iwasawa reviewed Jul 13, 2016
View reviewed changes

Set cleanup camera settings

3eeb02f

reverted default units

2e52c69

Fixed new scene having correct dpi even if default settings are set o…

ae0f0a6

…ther than 53.33333

hide fields for exportlevelpopup if pixelsonly

0278198

shun-iwasawa reviewed Jul 13, 2016
View reviewed changes

shun-iwasawa merged commit 535d3f2 into opentoonz:master Jul 14, 2016

ghost deleted the pixels branch August 4, 2016 05:28

Add pixels - Finished. #400

Add pixels - Finished. #400

Conversation

ghost commented Jun 4, 2016 • edited by ghost Loading

ghost commented Jun 4, 2016

shun-iwasawa commented Jun 6, 2016 • edited Loading

ghost commented Jun 6, 2016 • edited by ghost Loading

ghost commented Jun 6, 2016

ghost commented Jun 6, 2016

shun-iwasawa commented Jun 6, 2016 • edited Loading

ghost commented Jun 9, 2016

shun-iwasawa commented Jun 15, 2016

ghost commented Jun 15, 2016

ghost commented Jun 15, 2016

shun-iwasawa commented Jun 15, 2016

shun-iwasawa commented Jun 15, 2016

ghost commented Jun 19, 2016

ghost commented Jun 19, 2016

shun-iwasawa commented Jun 27, 2016 • edited Loading

shun-iwasawa Jun 27, 2016

Choose a reason for hiding this comment

shun-iwasawa commented Jun 27, 2016

ghost commented Jun 27, 2016

ghost commented Jun 30, 2016

ghost commented Jul 13, 2016

ghost commented Jul 13, 2016

shun-iwasawa Jul 13, 2016

Choose a reason for hiding this comment

ghost commented Jul 13, 2016

shun-iwasawa commented Jul 13, 2016

ghost commented Jul 13, 2016

shun-iwasawa commented Jul 13, 2016 • edited Loading

ghost commented Jul 13, 2016

shun-iwasawa commented Jul 13, 2016 • edited Loading

ghost commented Jul 13, 2016 • edited by ghost Loading

shun-iwasawa commented Jul 13, 2016

ghost commented Jul 13, 2016 • edited by ghost Loading

ghost commented Jul 13, 2016 • edited by ghost Loading

ghost commented Jul 13, 2016

shun-iwasawa commented Jul 13, 2016

ghost commented Jul 13, 2016

shun-iwasawa commented Jul 13, 2016

shun-iwasawa Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

shun-iwasawa commented Jul 13, 2016

ghost commented Jul 14, 2016

shun-iwasawa commented Jul 14, 2016

ghost commented Jul 14, 2016

ghost commented Jun 4, 2016 •

edited by ghost

Loading

shun-iwasawa commented Jun 6, 2016 •

edited

Loading

ghost commented Jun 6, 2016 •

edited by ghost

Loading

shun-iwasawa commented Jun 6, 2016 •

edited

Loading

shun-iwasawa commented Jun 27, 2016 •

edited

Loading

shun-iwasawa commented Jul 13, 2016 •

edited

Loading

shun-iwasawa commented Jul 13, 2016 •

edited

Loading

ghost commented Jul 13, 2016 •

edited by ghost

Loading

ghost commented Jul 13, 2016 •

edited by ghost

Loading

ghost commented Jul 13, 2016 •

edited by ghost

Loading

shun-iwasawa Jul 13, 2016 •

edited

Loading