Keep camera on reload

[snoyer]

Felt bad opening an issue without at least looking into it a bit so here's an attempt at reloading the current file without resetting the camera.

The use case is:

1. loading a procedural or otherwise generated model, navigating around
2. externally regenerating the model
3. reloading the model in F3D keeping the current view.

This implementation is based on what's been discussed on Discord:

1. adding a mechanism to save and restore camera states by key. I feel like this would be better using opaque state objects but I could not get it to work :(
2. saving the camera state before reloading, restoring afterwards before the next render

I don't know if this behaviour should be the default or not?
If not, shift+up could be used, but do we have access to modifiers in interactor callbacks?
The type of model/scene could also play a role maybe (eg. only reset the camera if the file defines cameras)?

[mwestphal]

but do we have access to modifiers in interactor callbacks?

Yes we do. But I think behavior could be default :)

[snoyer]

I added deleteState to make the state management complete (could not delete without restoring before), so the API is now:

• CameraStateKey saveState()
• bool restoreState(const CameraStateKey&)
• bool deleteState(const CameraStateKey&)
• with an internal map<CameraStateKey, ...> storing the states (as vtkCamera deep copies)

Using opaque state objects would allow to delegate the storing of the states (which would then be optional, and not needed for this PR for example) to the app and simplify the API to:

• CameraState saveState()
• void restoreState(const CameraState&)

However my lack of c++ skills is standing in the way of me implementing it that way. Options are then:

1. we want opaque state object because it's nicer and:
   a. I keep banging my head against it until it works
   b. someone more competent implements it instead
2. we leave it as is (using keys and internal management)

Thoughts?

[mwestphal]

I do think storing the states in the App would be better, but I'm not sure I will be able to help shortly. Could you push a branch showing what did not work for you ?

[snoyer]

Could you push a branch showing what did not work for you ?

Basically I have a class cameraState_impl : public cameraState to hold and hide a vtkCamera, so far so good.
The problem was I was trying to do:

• cameraState saveState(){ return new cameraState_impl(...); }
• restoreState(cameraState&){ ... dynamic_cast<cameraState_impl&>(cameraState) ... }
  Which, according to the internet, wasn't going to work because polymorphism without pointers just isn't a thing (for reason I can't be bothered to dig into really). Compiler was fine building it all only for it to bad_cast at runtime though, fun times!

Using pointers (CameraState* saveState(), void restoreState(CameraState*)) works. Maybe using smart pointers would be better but I'm not fluent enough to tell.

[github-actions]

You are modifying libf3d public API! ⚠️Please update bindings accordingly⚠️!
You can find them in their respective directories: python, java, webassembly.

[mwestphal]

Let me know if you need help with this @snoyer , the release rush is running out.

I'm not available on discord for a few days though.

[snoyer]

Let me know if you need help with this @snoyer

Thanks! so far it's working as a proof of concept, it does solve the use case that is.

Having the camera state as a pointer feels off though, I guess it'd be fine within the app code but what if it needs to be exposed through bindings? (don't really want to have to delete from Python for example).
Again, I'm not fluent enough in c++ to know where to go from there (smart pointers? wrap the raw pointer in a class, which is probably reinventing smart pointers? Leave is as is and change it later if needed, it's an opaque object so that'd be fine? ...). You'll have to take a look whenever you've got time and make a design decision

[mwestphal]

Could you give a go to the implementation with an opaque object and I can try to make it work ?

[snoyer]

Could you give a go to the implementation with an opaque object and I can try to make it work ?

That's where's it at now, cameraState_impl holds a vtkCamera to be used internally, and the exposed cameraState just gets passed around exposing nothing:

f3d/library/private/camera_impl.h

Lines 23 to 28 in 5416af1

	class cameraState_impl : public cameraState
	{
	public:
	vtkNew<vtkCamera> Camera;
	cameraState_impl(vtkCamera* vtkCamera) { this->Camera->DeepCopy(vtkCamera); }
	};

f3d/library/public/camera.h

Lines 12 to 16 in 5416af1

	class F3D_EXPORT cameraState
	{
	public:
	virtual ~cameraState() = default;
	};

[mwestphal]

ok ! Could you rebase your branch on master ? I will then take a look this week I hope

[mwestphal]

Just tested and it seems to work fine. Am I missing anything ?

[mwestphal]

BTW I wonder if we should add a modifier to keep cameraState when switching between files, it seems like a usefull feature.

[snoyer]

Just tested and it seems to work fine. Am I missing anything ?

It is functional.
But CameraState* saveState(), void restoreState(CameraState*) feels... meh? I believe (maybe wrongly) that CameraState saveState(), void restoreState(CameraState&) (no pointers) would be better. Thoughts?

BTW I wonder if we should add a modifier to keep cameraState when switching between files, it seems like a usefull feature.

1. See first post: need to pass modifiers to callbacks then
   

   f3d/library/private/interactor_impl.h

   Line 37 in e035ec3

   interactor& setKeyPressCallBack(std::function<bool(int, std::string)> callBack) override;

   
   

   f3d/library/src/interactor_impl.cxx

   Line 86 in e035ec3

   if (self->KeyPressUserCallBack(keyCode, keySym))

2. Would need to specify the expected behavior for edge cases. I did the easy thing of copying the vtkCamera which works just fine for a single model by restoring the exact same absolute camera position. However when going across models that are centered/oriented differently we may need to save/restore the F3D camera parameters instead of the current VTK camera.

[mwestphal]

Ok, got it, Ill take a look.

[mwestphal]

However when going across models that are centered/oriented differently we may need to save/restore the F3D camera parameters instead of the current VTK camera.

I think I'll go this way, and we will not need any opaque object :)

[mwestphal]

Here it is @snoyer : #788.

Can I let you finish it ?

Regarding the modifier, you are absolutely right, I will look into adding an API for modifiers in the callbacks.

[snoyer]

Here it is @snoyer : #788.

Can I let you finish it ?

I'm going to need to learn myself some more git first to rebranch from yours but I'll see what I can do :)

Regarding the modifier, you are absolutely right, I will look into adding an API for modifiers in the callbacks.

Careful you don't run into #443 when you go there, there's no telling how deep it could drag you ;p

[mwestphal]

Yeah, I'll avoid #443 for now.

some more git first to rebranch from yours but I'll see what I can do

Here is how I would do it:

git remote add mwestphal https://github.com/mwestphal/f3d.git
git fetch mwestphal
git cherry-pick 64c38377cbff0c3d502bcef41cad48e18346ccf8

hth,

[snoyer]

Are we looking at restoring the camera on previous/next model in this PR or just on reloading the current model?

If we want to do prev/next we need to first assess and decide whether the camera state should be:

1. absolute in space (cam position, focal point, ...), or
2. relative to the models (azimuth, elevation, ...)

[mwestphal]

imo a camera state should be absolute, so I'd focus on that first. If we get fancy we camera positions in the future, we may want to revisit it at some point, but for now seems good enough.

So lets only focus on keeping camera location on the UP interaction for now with an absolute position :)

[mwestphal]

Need any help with this one @snoyer ?

[mwestphal]

closing in favor of: #705