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

[3d] export all frames from QGIS 3d animations as images #9244

Merged
merged 5 commits into from
Feb 27, 2019

Conversation

PeterPetrik
Copy link
Contributor

fix #21300

Now it is possible to export frames of 3d animation as png/jpg/... images. These can be easily merged into .avi (outside QGIS)
screenshot 2019-02-22 at 15 33 58

screenshot 2019-02-22 at 15 41 04

@PeterPetrik PeterPetrik added this to the 3.8 milestone Feb 22, 2019
@nirvn
Copy link
Contributor

nirvn commented Feb 22, 2019

Nice. A few comments:

  • spell out FPS , ie frame per second, to make that setting clearer to the average user (you have the space anyways)
  • move the width and height unit into the spinbox like it's done in other parts of our UI (eg, save canvas as image dialog)
  • while your at it, I'd rename width and height to output width and output height

Very nice to see this feature, thanks!

@nirvn
Copy link
Contributor

nirvn commented Feb 22, 2019

Oh and maybe add an hint label below the template line edit to explain the use of #s?

src/app/3d/qgs3danimationwidget.cpp Outdated Show resolved Hide resolved
src/ui/3d/animationexport3ddialog.ui Outdated Show resolved Hide resolved
@nyalldawson
Copy link
Collaborator

The "#####" placeholder looks very deceptive to me - if I read the code correctly it needs to be present in its entirety, but it looks to users like ## should be allowed for 2 digit frames, ### for 3, etc. (Shouldn't this be supported anyway?)

@nirvn
Copy link
Contributor

nirvn commented Feb 23, 2019

+1 on what @nyalldawson raised. The template should handle a varying nb of #s. The more #, the more leading zeroes.

Eg:

  • myanim-# would result in myanim-1, ... myanim-10, etc
  • myanim-### would result in myanim-001, ... myanim-010, etc

@nirvn
Copy link
Contributor

nirvn commented Feb 24, 2019

Actually, on the template front, why don't we use our expression engine instead (like we do for atlas export file names)? We could pass the frame number as a @current_frame variable.

That'd make it better match the UX with other parts of QGIS and be a lot more flexible.

@nirvn
Copy link
Contributor

nirvn commented Feb 24, 2019

@total_frame would be useful too for filenames like anim-001-350.png, anim-002-350.png, etc.

@NathanW2
Copy link
Member

NathanW2 commented Feb 24, 2019 via email

@PeterPetrik
Copy link
Contributor Author

@nirvn I have spend several hours on the most difficult problem in the software development world. When thinking about expressions in this field, I ended up with
[% @project_basename %]-[% format_number(@current_frame, 0, 5) %]-[% @total_frames %].png

and compared to
abc#####.png

I sadly concluded that I prefer to stick with the original approach.

Said that, the problem with expressions would be also formatting (0001, 0002), since user would need to insert another function to left-pad it.

@PeterPetrik PeterPetrik added the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Feb 25, 2019
src/3d/qgs3dutils.cpp Outdated Show resolved Hide resolved
src/3d/qgs3dutils.cpp Outdated Show resolved Hide resolved
src/app/3d/qgs3danimationwidget.cpp Outdated Show resolved Hide resolved
src/ui/3d/animationexport3ddialog.ui Outdated Show resolved Hide resolved
src/ui/3d/animationexport3ddialog.ui Outdated Show resolved Hide resolved
@roya0045
Copy link
Contributor

@PeterPetrik What about :
[% @project_basename %]-[% lpad(to_string(@current_frame),length(to_string(@total_frames))) %]-[% @total_frames %].png

@PeterPetrik PeterPetrik added Squash! Remember to squash this PR, instead of merging or rebasing and removed Requires Tests! Waiting on the submitter to add unit tests before eligible for merging labels Feb 26, 2019
@PeterPetrik
Copy link
Contributor Author

@roya0045 good idea, but I may need to buy a second monitor to be able to see the full text :)

@nirvn
Copy link
Contributor

nirvn commented Feb 26, 2019

@PeterPetrik , if you look at the way the filename is handled in the context of atlas, open the layout dialog and pop the atlas panel. It does not use the [% %] expression brackets as the whole string is the expression, which makes things less chaotic (as you are pointing out above).

The default expression there is: 'output_'||@atlas_featurenumber'.

I see no problem in have a default template string here being: '3d_'||@current_frame'.

In terms of how you serve @current_frame, you could either pass on an integer value, or a string with padded 0s (the latter would be friendlier). If someone wants the int value, he/she could do to_int(@current_frame).

@wonder-sk
Copy link
Member

@nirvn first I got also excited by the idea to use expressions for the filename template. But then I realized that a default like '3d_'||@current_frame||'.jpg' is quite cumbersome and would need @current_frame to be a string. And it gets more complicated:

  • how many leading zeros to use for @current_frame? people get very opinionated about such thing :)
  • doing some arithmetic with @current_frame is cumbersome - e.g. lpad(to_int(@current_frame)+100, 5, '0')

It seems to me that while expressions are useful for power users, for an average person who just wants to get animation exported the default filename template would be a bit cryptic wouldn't it? To me it seems that something like 3d-####.jpg is pretty self-explanatory and easy to adjust.

And do we even need fancy expression stuff here? The main point is to export frames and turn them into a video in the next step, so in theory filenames should be quite irrelevant :)

@nirvn
Copy link
Contributor

nirvn commented Feb 26, 2019

@wonder-sk ,

  • I'm not sure how cumbersome it is, that's a subjective call 😉
  • On @current_frame being a string, I don't see this as being a problem; as I was saying, if anyone wants a number, he/she can use to_int(...)
  • On the leading zeros issue, IMHO, the default should be as many leading zeros as you need to pad the current frame to match the width of your total frame (i.e., for 30 total frames, it should be 01, 02; for 300 total frames, it should be 001, 002)

That said, this ain't an hill I want to die on 😉 I think it'd be useful, but if you guys have a strong feeling on this issue, go with your favorite implementation method.

@nirvn
Copy link
Contributor

nirvn commented Feb 26, 2019

@wonder-sk , @PeterPetrik , BTW, I assume the file extension in the template string determines the image format used to export frames. Is that so? That might be worth separating into its own setting, and presented as a combo box of formats.

@PeterPetrik
Copy link
Contributor Author

  • extension is anything that QImage can export to and not sure how would be possible to create such list easily. Also as a separate combobox it would have more sense in combination with expressions.

  • with the expression vs #### discussion, I do not have strong feeling about any implementation, but my preference is for simpler #### form for now (also in respect of the fact that I have had both implementations already implemented and spend several hours yesterday going back and forth :) ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Squash! Remember to squash this PR, instead of merging or rebasing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants