Initial editor #49

MarcusAndreasSvensson · 2023-11-10T17:25:05Z

Hey,

I made a potential initial version of an editor class and added a tool for acquiring the up vector by selecting three points.
Plan is to add more tool for editing the scene further.

Out of pure necessity for my old computers sake I added an option for an on-demand frame loop which is updated with the invalidate method. Attached the invalidation to the controls and some other events to not be noticeable.

Please let me know if you would like code to be structured in some other way.

mkkellogg · 2023-11-10T18:27:22Z

Cool! I've been thinking about adding some editing functionality for a while; I'll take a look!

mkkellogg · 2023-11-11T00:30:49Z

I'm still looking things over but I do have a few thoughts I want to share up-front.

I do want to transition to more of a proper release-based approach to development, which means I would like to only merge features into main that are fairly complete (and I'm thinking about a separate dev branch as well). I know development on something like an editor will be more open-ended, but I think we can break it up into smaller, well defined features. So in this case it might make sense to create another feature branch off of main in this repo, and we can work on that until we think it's ready to be merged into main.

I really like the idea of selecting three points to define a camera-up vector, but I think at the very least we need to have a nicely styled menu and some visuals/3D-markers in the scene that indicate where the user has selected those points. I also think that we need a better way to override the handling of mouse and keyboard events in the Viewer, which makes me think that Editor might be better off inheriting from Viewer (although I'm still thinking about that).

Another thing to keep in mind is that we're still trying to figure out a good format for .splat files and I think users will have the expectation to save any changes they make in the editor to a file. I'm not sure yet if we should hold off on releasing any editor-related stuff until that format is more finalized (or maybe we just make it clear to the user that changes made in the editor cannot be saved for now).

Since the editor will probably be a fairly large feature (eventually), I just want to make sure we think about these kinds of things ahead of time and get the design right (or close) up-front.

mkkellogg

Finished an initial pass, let me know your thoughts.

mkkellogg · 2023-11-11T00:59:15Z

.gitignore

@@ -1,3 +1,5 @@
 dist
 build
 node_modules
+.vscode/**/*
+demo/assets/data/**/*


Do we need to add this? The demo directory is really meant to be treated as a source code directory (which means maybe it should actually be moved under /src). Would it be possible to use build/demo/assets/data for the same purpose?

No, not really, I just didn't want to accidentally commit unnecessary files.

Not sure if it's appropriate with the build/demo/assets/data-route. I would say it depends on what the goal/vision is, which I can't say I see clearly.

mkkellogg · 2023-11-11T00:59:43Z

demo/index.html

      const viewer = new GaussianSplats3D.Viewer(viewerOptions);
+
+      let editor;


Is this variable used anywhere?

Well no, not really. This would be a non-issue if we were to go with the inheritance route.

mkkellogg · 2023-11-11T01:00:48Z

demo/index.html

      const viewerOptions = {
        'cameraUp': cameraUpArray,
        'initialCameraPosition': cameraPositionArray,
-        'initialCameraLookAt': cameraLookAtArray
+        'initialCameraLookAt': cameraLookAtArray,
+        'frameloop': 'demand',


I think we should make this an enum.

mkkellogg · 2023-11-11T01:02:52Z

src/Editor.js

+
+    const planeFinderLabel = document.createElement('p');
+    planeFinderLabel.id = 'planeFinderLabel';
+    planeFinderLabel.innerHTML =


I see this code in multiple places, maybe it's worth making an updateCameraUpLabel() function (or something like that).

Yes, there are a few places where we could refactor to separate functions

mkkellogg · 2023-11-11T01:06:11Z

src/Editor.js

+
+  setupPlaneFinder() {
+    if (this.viewer.useBuiltInControls) {
+      this.viewer.rootElement.removeEventListener('pointerup', this.viewer.pointerUpHandler);


I definitely think we need a less intrusive way of overriding behavior in Viewer. I'm still not sure we should simply have Editor inherit from Viewer and just override the onMouseDown() etc. functions, or if we should add functions to Viewer to allow them to be set by an external source.

I'm mainly a React developer and am not all that familiar with best practices in vanilla and OOP.This could be a case of lack of knowledge on my part.

The effect I was going for was to have a default global state that is the same as just the viewer. I couldn't really figure out how to do it with inheritance + overriding without loosing the default state. In my mind we had to store those handlers away anyhow and I figured that we might aswell keep the class itself intact as that stored default state.

So I was thinking something along the lines of:

class Viewer { constructor() { window.addEventListener("mousedown", this.onMouseDown.bind(this)); } onMouseDown() { // default behavior } } const PointerMode = { Default: 0, CameraUpSelection: 1 }; class Editor extends Viewer { constructor() { super(); this.pointerMode = PointerMode.Default; this.setupEditPanel(); } setupEditPanel() { const planeFinderButton = document.createElement('button'); planeFinderButton.innerHTML = 'Find ground plane'; planeFinderButton.addEventListener('click', () => { this.pointerMode = PointerMode.CameraUpSelection; }); } // override Viewer.onMouseDown() onMouseDown() { if (this.pointerMode === PointerMode.CameraUpSelection) { // Do custom stuff for camera up selection } else { super.onMouseDown(); } } }

This would retain default behavior when not selecting the camera-up vector. (This code probably contains errors, but hopefully it conveys the idea :) )

Yes, this is absolutely more clear, I like it.

With this structure I would prefer to inherit the Viewer class.

I also think a user should be able to jack into the events and extend the functionality in a straight forward manner. I'm not really sure how one would do that right now, maybe something like how I bound the invalidate() method to the controllers change event. Is there something with this approach that would hinder that?

I don't think an inheritance based approach will hinder that; we can still extend functions in Viewer by overriding them with custom functionality and making sure to call the base function along with it. We have more options that way because we can override any function and it's cleaner because it doesn't require changes to Viewer. I do agree that it would be nice for Viewer to have a general way to hook into events so that anyone developing with the library could hook into them. Maybe we need to make Viewer extend an EventDispatcher class and emit events at the appropriate places.

+1 on making Viewer extend EventDispatcher

mkkellogg · 2023-11-11T01:18:43Z

src/Editor.js

+    return function(mouse) {
+      clickOffset.copy(this.viewer.mousePosition).sub(this.viewer.mouseDownPosition);
+      const mouseUpTime = getCurrentTime();
+      const wasClick = mouseUpTime - this.viewer.mouseDownTime < 0.5 && clickOffset.length() < 2;


I think we need to change Viewer.onMouseUp() to only do these calculations to determine if there was a click, and define a new function Viewer.onClick() and call that from Viewer.onMouseUp(). Then we could just override Viewer.onClick() and not have to replicate the click detection logic.

mkkellogg · 2023-11-11T01:19:34Z

src/Viewer.js

@@ -125,11 +134,13 @@ export class Viewer {
            this.controls.maxPolarAngle = Math.PI * .75;
            this.controls.minPolarAngle = 0.1;
            this.controls.enableDamping = true;
-            this.controls.dampingFactor = 0.05;
+            this.controls.dampingFactor = 0.25;


Any particular reason for this change? If the editor needs higher damping, maybe we can make it a parameter to the Viewer.

When the damping is a little lower the "demand" rendering has a much quicker effect. I little of a personal preference, but I also think it's easier to work with in an edit mode, when you want more control of the angles. With that said, this change should maybe be inside the Editor class if we decide to keep it.

Regarding if we should make it a parameter to the Viewer class I think we can keep it as it is and let the users reach into the viewer to modify the controls and camera. In my experience it's pretty common practice when working with three. Maybe we could mark them as public for clarity.

Agreed that this should be set from within the Editor class. Maybe a parameter to the Viewer class doesn't quite make sense. Also, we should be careful about creating too many parameters for Viewer

mkkellogg · 2023-11-11T01:20:12Z

src/Viewer.js

@@ -125,11 +134,13 @@ export class Viewer {
            this.controls.maxPolarAngle = Math.PI * .75;
            this.controls.minPolarAngle = 0.1;
            this.controls.enableDamping = true;
-            this.controls.dampingFactor = 0.05;
+            this.controls.dampingFactor = 0.25;
+            this.controls.zoomToCursor = true;


I'm also curious why this setting was changed.

Same thing as previous, a little more control when trying to get specific angles when editing. Also this one should be inside the Editor class though.

Could we make a toggle in the Editor UI for this?

mkkellogg · 2023-11-11T01:22:37Z

src/Viewer.js

+        this.loadingSpinner = new LoadingSpinner();
+        this.loadingSpinner.hide();
+
+        this.viewerNeedsUpdate = true;


Could we call this flag viewNeedsRefresh ? Just so it's clear that just the view/screen needs to be updated.

Sure, I was just kind of going for the "viewMatrixNeedsUpdate"-feel, but I don't have any hard stance on either

MarcusAndreasSvensson · 2023-11-11T15:50:51Z

A thought about the ability to edit splats and saving the result.

I do think we can go ahead and do the exports just as you can do on the starting page. If we decide to not save changes in the editor I think we should also remove that functionality from the start page and keep them in sync.

At the very least I think it should be exportable to the original .ply format, cause that's going to be somewhat relevant for a while.

MarcusAndreasSvensson · 2023-11-11T15:53:54Z

Adding to the UI/UX concerns you lifted about the up-finder. I've been thinking about adding a button to flip the normal, as it is sensitive to the winding of the points which might not be clear for everyone.

mkkellogg · 2023-11-11T16:40:45Z

Good call on consistency between exporting .splat files on the main demo page and from the editor. At this point I suppose we can just add the same exporting capability to the editor. When the time comes to implement the universal .splat format we're currently designing we'll make sure to notify users (or maybe even retain support for importing the old format?).

As for flipping the camera-up normal, I think a button is a good idea. I didn't even think about winding-order problems :)

MarcusAndreasSvensson · 2023-11-11T17:21:44Z

I think it would be a good idea to keep support for the older export. If we decide to do that we should somehow store version info in a header or something in the file.

To me it feels a little over the top when the format is not set at all. On the other hand I would get very annoyed if my previously exported file would work nowhere all of a sudden with no option to re-export it to a newer format.

edit:
I see I contradict myself in this comment, and it reflects my feelings about this pretty closely

MarcusAndreasSvensson · 2023-11-13T18:30:08Z

I feel we have something to work with now here after all discussions. I'm going to sit down and work on an updated version in 1 or 2 days and go for a second round of review.

Anything you'd like to add before I dig in?

mkkellogg · 2023-11-14T04:29:42Z

Nothing comes to mind right now, I'll let you know if I think of anything :)

MarcusAndreasSvensson added 2 commits November 10, 2023 19:13

initial editor class + on demand rendering

d723d79

initial editor

c9b7b2d

MarcusAndreasSvensson force-pushed the editor branch from a92e5f4 to c9b7b2d Compare November 10, 2023 18:15

mkkellogg reviewed Nov 11, 2023

View reviewed changes

mkkellogg force-pushed the main branch from d67297b to 369425e Compare March 11, 2024 18:30

		const viewer = new GaussianSplats3D.Viewer(viewerOptions);

		let editor;

Initial editor #49

Are you sure you want to change the base?

Initial editor #49

Conversation

MarcusAndreasSvensson commented Nov 10, 2023

mkkellogg commented Nov 10, 2023

mkkellogg commented Nov 11, 2023

mkkellogg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcusAndreasSvensson commented Nov 11, 2023

MarcusAndreasSvensson commented Nov 11, 2023

mkkellogg commented Nov 11, 2023

MarcusAndreasSvensson commented Nov 11, 2023 • edited

MarcusAndreasSvensson commented Nov 13, 2023

mkkellogg commented Nov 14, 2023

MarcusAndreasSvensson commented Nov 11, 2023 •

edited