Skip to content

VIDEO: Add quality, warpMode and swing transition in QTVR panorama #6548

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

VIDEO: Add quality, warpMode and swing transition in QTVR panorama #6548

wants to merge 1 commit into from

Conversation

Malharbdv
Copy link
Contributor

Add the following missing features in QTVR panorama as described in the original director QTVR documentation

  • 4 levels of quality mode, also added the dynamic quality where the quality is changing based on whether the user is actively intereacting with the panorama
  • swing transition from the current node to the target node in a smooth animation
  • 3 levels of warping

@Malharbdv
Copy link
Contributor Author

Guys, I'm not sure why the check failed, can you guys help me out?

@bluegr
Copy link
Member

bluegr commented Apr 8, 2025 •
edited
Loading

Guys, I'm not sure why the check failed, can you guys help me out?

It's an issue with the runner images, which are using the latest versions in all packages, and the CMake package had a breaking change in version 4. Rebase your branch, so that you can get this commit from master, which fixes the issue: 2821fcd

@sev-
Copy link
Member

sev- commented Apr 10, 2025

Two things right away:

  • VIDED -> VIDEO in the commit log message
  • Split director change into a separate commit

@Malharbdv Malharbdv changed the title VIDED: Add quality, warpMode and swing transition in QTVR panorama VIDEO: Add quality, warpMode and swing transition in QTVR panorama Apr 10, 2025
@Malharbdv Malharbdv force-pushed the qtvr_xtra branch 2 times, most recently from 93d3d06 to b46849d Compare April 10, 2025 14:23
@Malharbdv
VIDEO: Add quality, warpMode and swing transition in QTVR panorama
91eb7f6 
Add the following missing features in QTVR panorama as described
in the original director QTVR documentation
- 4 levels of quality mode, also added the dynamic quality
  where the quality is changing based on whether the user
  is actively intereacting with the panorama
- swing transition from the current node to the target node
  in a smooth animation
- 3 levels of warping
@Malharbdv
Copy link
Contributor Author

I made the changes, pushed again, but the check failed once again
I read the error message, I resolved it, I squashed the last three commits and pushed again

@sev- sev- added the GSoC Part of a Google Summer of Code project label Apr 11, 2025
sev-
sev- requested changes Apr 11, 2025
View reviewed changes
Copy link
Member

@sev- sev- left a comment

Choose a reason for hiding this comment

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

Reviewed partially

video/qt_decoder.h
@@ -206,6 +212,10 @@ class QuickTimeDecoder : public VideoDecoder, public Audio::QuickTimeAudioDecode
kUpdateModeOffscreenOnly,
kUpdateModeFromOffscreen,
kUpdateModeDirectToScreen,

kScaleFactorOne = 1,
Copy link
Member

Choose a reason for hiding this comment

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

I do not like this enum usage. It is not consistent. You are passing it as an integer but then work with the actual values. I would suggest dropping it altogether and using numbers.

video/qt_decoder.h

kScaleFactorOne = 1,
kScaleFactorTwo = 2,
kScaleFactorThree = 3,
Copy link
Member

Choose a reason for hiding this comment

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

We were discussing that possible difference between "low quality" antialasing and high quality could be with linear/gaussian interpolation. How is this approach of 2x/3x is compared to this and the original?

video/qt_decoder.h
@@ -418,10 +429,17 @@ class QuickTimeDecoder : public VideoDecoder, public Audio::QuickTimeAudioDecode
Graphics::Surface *_projectedPano;
Graphics::Surface *_planarProjection;

// Defining these for the sake of making the swing transition happen
// This will be the start point of the transition
float _currentFOV = 0;
Copy link
Member

Choose a reason for hiding this comment

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

These seem to be an artifact from the previous approach of external looping. I think you can kill them and use local variables.

video/qt_decoder.h
bool _dirty;
short _counter = 10;
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here, now you have a loop which you execute start to finish, so no need to keep it in the object state.

video/qtvr_decoder.cpp
@@ -43,6 +43,7 @@
#include "common/timer.h"
#include "common/util.h"

#include "common/events.h"
Copy link
Member

Choose a reason for hiding this comment

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

Please put it higher so it is sorted alphabetically

video/qtvr_decoder.cpp
@@ -333,25 +357,30 @@ bool QuickTimeDecoder::setFOV(float fov) {
PanoSampleDesc *desc = (PanoSampleDesc *)_panoTrack->sampleDescs[0];
bool success = true;

if (fov == 0.0f) // No change
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be gone now. Probably an oversight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure, whether this should be there or not
Why do we return if the fov is set at 0?

video/qtvr_decoder.cpp
@@ -462,7 +491,6 @@ void QuickTimeDecoder::goToNode(uint32 nodeID) {
}

_currentSample = idx;

Copy link
Member

Choose a reason for hiding this comment

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

Consider reverting. It is good to split logical blocks vertically.

video/qtvr_decoder.cpp
@@ -474,7 +502,8 @@ void QuickTimeDecoder::goToNode(uint32 nodeID) {
// PANO Track
////////////////////////

QuickTimeDecoder::PanoTrackHandler::PanoTrackHandler(QuickTimeDecoder *decoder, Common::QuickTimeParser::Track *parent) : _decoder(decoder), _parent(parent) {
QuickTimeDecoder::PanoTrackHandler::PanoTrackHandler(QuickTimeDecoder *decoder, Common::QuickTimeParser::Track *parent)
Copy link
Member

Choose a reason for hiding this comment

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

Please revert.

video/qtvr_decoder.cpp
// TODO: To create an enum for all the quality types
switch ((int) quality) {
case 1:
projectPanorama(kScaleFactorOne, fov, hfov, tiltAngle, panAngle);
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is the place where I suggest using just the numbers instead of enum.

video/qtvr_decoder.cpp
break;

case Common::EVENT_QUIT:
projectPanorama(kScaleFactorThree, _decoder->_fov, _decoder->_hfov, _decoder->_panAngle, _decoder->_panAngle);
Copy link
Member

Choose a reason for hiding this comment

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

No, you need to quickly quit the execution here, without re-rendering, as this is a request to quit ScummVM completely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sev- sev- sev- requested changes

Assignees
No one assigned
Labels
GSoC Part of a Google Summer of Code project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Malharbdv @bluegr @sev-