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

camera layer problem #940

Closed
tirbo opened this Issue Apr 11, 2018 · 32 comments

Comments

Projects
None yet
5 participants
@tirbo
Copy link

tirbo commented Apr 11, 2018

Ubuntu 16.04
Любая ночная сборка.
При работе со слоем камеры. Если использовать одновременно сдвиг и масштаб то сохранения работают некорректно.
Any night build.
When working with the camera layer. If you use both the shift and the scale, then the save does not work correctly.

@chchwy chchwy added the bug label Apr 11, 2018

@chchwy chchwy self-assigned this Apr 11, 2018

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Apr 12, 2018

Hi @tirbo
Can you give a specific example?
I did a quick test in 11 April 2018 Nightly build the shfit & scale is working.

@chchwy chchwy added the more info label Apr 12, 2018

@tirbo

This comment has been minimized.

Copy link
Author

tirbo commented Apr 12, 2018

Я сейчас на работе. Тяжело объяснить в письме. Тут нужно сделать видео. Но все же попробую объяснить. Для примера. Нарисуйте любой объект в левом верхнем углу рабочего пространства. Далее перейдите в слой камеры. Добавьте 10 кадр, тут переместите объект в нижний левый угол. Добавьте 20 кадр... И так по периметру верните объект на место. Сохраните проект. Закройте программу. Откройте программу и проект. Пока все работает корректно. Теперь снова работаем в слое камеры. Переходим на 10 кадр и изменяем масштаб объекта. Переходим на 30 кадр и снова изменяем масштаб объекта (за счет приближения камеры). Смотрим получившуюся анимацию.
Сохраняем проект. Открываем проект. И видим что анимация изменилась. У меня объект теперь ходит по диагонали а не по периметру. Ну если у вас все будет нормально я сделаю видео и дам ссылку на ютуб.
I'm at work. It is difficult to explain in the letter. Here me need to make a video. But still I'll try to explain. For example. Draw any object in the upper left corner of the workspace. Then go to the camera layer. Add 10 frames, then move the object to the bottom left corner. Add 20 frames ... And so on the perimeter, return the object to its place. Save the project. Close the program. Open the program and project. While everything works correctly. Now we are working again in the camera layer. Go to frame 10 and change the scale of the object. We go to frame 30 and again change the scale of the object (due to the approach of the camera). We look at the resulting animation.
Save the project. We open the project. And we see that the animation has changed. My object now moves diagonally and not along the perimeter. Well, if everything goes well with you, I'll make a video and give you a link to YouTube

@tirbo

This comment has been minimized.

Copy link
Author

tirbo commented Apr 12, 2018

video

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Apr 12, 2018

@tirbo @chchwy I tried to follow tirbo's instructions as close as I could but I could not replicate this issue on Windows 7. It's probably a Linux specific issue, but a video would be appreciated to see how the test exposes the bug.

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Apr 12, 2018

@chchwy One thing I noticed when I did my own test was that when I added a keyframe inbetween two existing keyframes, the "path" of motion would change slightly, particularly when there was zooming. However the motion that follows the "perimeter" was saved properly in my own test.

@tirbo If possible can you upload and share the last file you made in the video to see if it opens properly on Windows but not in Linux?

@tirbo

This comment has been minimized.

Copy link
Author

tirbo commented Apr 12, 2018

@tirbo

This comment has been minimized.

Copy link
Author

tirbo commented Jul 17, 2018

проблема не будет исправлена?
The problem will not be fixed?

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Jul 17, 2018

@tirbo Hi. It's not that the problem won't be fixed, however all the current developers are volunteers and work when they can. Also they have either windows or mac so they would need to test a linux distro to reproduce your specific issue. There are also a lot of bug fixes in the queue but rest assured this will be fixed eventually and when it is I'll message you again, it will just take time.

With that said If you know someone who can code in C++ and want to help the project please send them here, even if it's just to fix this issue, otherwise we have to be patient while we wait for most fixes to be done.

If you require a way to handle your camera animation, maybe try to combine your Pencil2D animation with Synfig ( http://synfig.org) It's a program for 2D animation and has a lot of effects. since their developers speak russian you can surely get proper support in your native language meanwhile Pencil2D gets this problem fixed. 🙂

@davidlamhauge

This comment has been minimized.

Copy link
Contributor

davidlamhauge commented Dec 17, 2018

I tried this on my Ubuntu 18-04, which is very stable. The one I use for coding.
I can reproduce everything, up to the time where I load the scene third time, after it is scaled and Pencil2D is closed.
When I start Pencil2D it crashes third time when I want to open the file, with this message:
ASSERT: "scaling > 0" in file ../../pencil/core_lib/src/structure/camera.cpp, line 25
I am not using Linux appimage. I run it from inside Qt Creator, with the latest master-branch.

@davidlamhauge

This comment has been minimized.

Copy link
Contributor

davidlamhauge commented Dec 17, 2018

I have tried to install a regular Pencil2D install in Ubuntu 18.04. It generates the same errors as @tirbo shows us.
Then I tried to download the stable appimage nightly release from October 14th. It generates the same errors. Hmm... Is it a Linux thing?

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Dec 17, 2018

Sounds like a Linux specific issue. What version of Qt are you using? @davidlamhauge

@davidlamhauge

This comment has been minimized.

Copy link
Contributor

davidlamhauge commented Dec 17, 2018

@chchwy I use Qt Creator 4.8.0 based on Qt 5.12.0 on both Ubuntu 18.04 and macBook. I mostly code on Ubuntu.

@davidlamhauge

This comment has been minimized.

Copy link
Contributor

davidlamhauge commented Dec 18, 2018

@chchwy Here is a video that shows what happens, and the content of the *.pclx

https://youtu.be/jeqdVGN5SwQ
I just saw that the scaling was '1.5', only it was written like we write it in Denmark, which is '1,5', since we use comma as separator between number and decimals. Can that be the problem?

@davidlamhauge

This comment has been minimized.

Copy link
Contributor

davidlamhauge commented Dec 18, 2018

@chchwy It is definitely the danish number separator that messes things up.
I can see that @tirbo also has comma-separators in his *.pclx file.
Is this something we can do something about or is it a Qt issue?

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Dec 18, 2018

@davidlamhauge what happens if you change the scaling separator back to ".", save and retry?

@davidlamhauge

This comment has been minimized.

Copy link
Contributor

davidlamhauge commented Dec 18, 2018

@CandyFace I tried but I couldn't. It's a zipped filed, and I wasn't allowed to do that.
I changed my system language to UK English, and it saved with '.', and when I changed it back to danish it saved with ','.
As I write, tirbo has comma-separators too, so it must be the problem.

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Dec 18, 2018

Given that we know it's a zip container, you can just rename .pclx to .zip and unpackage it. Then do the same in reverse to change it to .pclx format again ;)

As for comma instead of dot, yeah that could definitely cause problems... it should be possible to force english locale on xml files to avoid this.

@davidlamhauge

This comment has been minimized.

Copy link
Contributor

davidlamhauge commented Dec 19, 2018

It seems like we have to set a global LC_NUMERIC to English like here: http://www.cplusplus.com/reference/clocale/setlocale/
It seems like the thing to do is:
std::setlocale(LC_NUMERIC, "En_US"); // in the start of the function
std::setlocale(LC_ALL, ""); reset locale at the end of the function

@davidlamhauge

This comment has been minimized.

Copy link
Contributor

davidlamhauge commented Dec 19, 2018

I finally had time to test your suggestion @CandyFace . I changed the scaling-separator to '.' (period) and everything worked fine. I also had to change the x and y positions since they are floats or doubles, so every number with decimals must be with period as separator.

CandyFace added a commit to CandyFace/pencil that referenced this issue Jan 25, 2019

chchwy added a commit that referenced this issue Feb 4, 2019

Merge pull request #1166 from CandyFace/iss940-fixed-locale-delimiter
#940 - Fix XML fields corrupting because of locale decimal types.

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

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Feb 10, 2019

@tirbo this should be fixed in latest nightly build, please check it out.
@davidlamhauge would you be able to verify too, I don't use danish locale myself so I've only been able to simulate the bug.

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Feb 18, 2019

The issue seems to happen only on Linux. I have switched to many different locales on Windows but didn't see the comma decimal separator.

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Feb 20, 2019

@chchwy @davidlamhauge @CandyFace Hey guys, quick update just happened to see that @tirbo posted a video on his youtube channel. Maybe to report this, but the bug is still NOT fixed in ubuntu 18.04.2 LTS. Honestly I don't understand why that keeps happening. He shows that he's using the latest February 19th 2019 APPIMAGE build.

https://www.youtube.com/watch?v=sVNiudXmfxQ

Edit: It's weird that it happens precisely on zoom, but not with translation.

tirbo could you please share the files before and after applying the zoom?

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Feb 20, 2019

Hmm... it's possible that the locale is being reset too early.. Maybe it should just be moved down to the bottom of the function...

@tirbo

This comment has been minimized.

Copy link
Author

tirbo commented Feb 20, 2019

Забыл пароль не мог войти. Видео есть. Вот файлы: МояАнимация_008.pclx МояАнимация_009.pclx
Forgot password could not enter. There is a video. Here are the files:***

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Feb 21, 2019

Probably the setlocal() doesn't change the behaviour of Qt XML deserialization. In my previous tests, no matter what locales I set (German or Denmark), I always got the period decimal separator.

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Feb 27, 2019

After digging into this issue a few hours, I found there could be no easy way to fix it as it relies on the Qt's XML implementation.

I will move this issue to v0.6.4. One possible solution is adopting a 3rd party XML library e.g., pugixml which apparently outstrips QDomDocument in both performance and memory consumption.

@chchwy chchwy modified the milestones: 0.6.3, 0.6.4 Feb 27, 2019

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Mar 17, 2019

Probably the setlocal() doesn't change the behaviour of Qt XML deserialization. In my previous tests, no matter what locales I set (German or Denmark), I always got the period decimal separator.

@chchwy If you tested this on mac os, you should know that changing the locale is not enough, mac os has a specific separator option under advanced in "Language and region" that handle this.
image

I forgot to answer but I tested a week ago my attempt to fix it and it didn't. So this is not resolved.
However Instead of relying on another dependency, another solution would be to make our own custom QDoubleSpinbox that is initialized with fixed QLocale.

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Mar 17, 2019

Mmm..I don't get it. Is QDoubleSpinbox related to the decimal separator of an xml file?

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Mar 17, 2019

No not the XML but the output of the value() of the widget that the XML file reads from. The input value is comma separated from the very beginning if your locale uses that. You can also see it directly in Pencil2D on those widgets affected.

Using comma as separator
image

using Dot as separator
image

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Mar 17, 2019

Oh I didn't know the issue is from a QDoubleSpinbox.
The camera position is controlled by hand tool. There souldn't be any x and y values from QDoubleSpinbox, right?

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Mar 17, 2019

Agh... I didn't think that through, yeah no of course we don't use qdoublespinboxes for the hand tool. What I meant is that you should be able to see the output value change based no the locale you have. So if you set the number separator to "," then it should show that in the debug log too.

I just tried again though but I can't seem to replicate the bug anymore.
Sigh... I guess the only way we can be sure is to have it confirmed by tirbo

@tirbo can you reproduce the problem in the latest release? (0.6.3)

@davidlamhauge

This comment has been minimized.

Copy link
Contributor

davidlamhauge commented Mar 17, 2019

I've just tested 0.6.3 on Ubuntu 18.04, and the problem persist. Unfortunately.

CandyFace added a commit to CandyFace/pencil that referenced this issue Mar 18, 2019

@chchwy chchwy closed this in 2402f66 Mar 20, 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.