Save current state to split files #267

klingbolt · 2024-01-20T15:11:26Z

Changes to markers are saved in the split files.

The next PR will save the splits to individual packs.

Spatial.gd

Icon.gd

Route.gd

AsherGlick · 2024-01-20T23:00:49Z

Spatial.gd

-	return data_out
+func save_current_map_data():
+	var packed_bytes = self.waypoint_data.to_bytes()
+	if packed_bytes.size() > 0:


Does this check prevent saving a file which went from having "some markers" to "no markers" ?
IE: prevent deleting the last marker on a map?

I was using the check to not make a blank file but you are correct it would prevent that. I'm going to remove the check.

Make and link issue to track this theoretical case of creating blank files. Because that is possible, maybe the correct logic here will be to delete files that are empty instead of not saving/overwriting them. But something a future pass can deal with.

Spatial.gd

AsherGlick · 2024-01-20T23:02:37Z

Spatial.tscn

+margin_left = 777.0
+margin_top = 89.0
+margin_right = 1448.0
+margin_bottom = 587.0


What is the motivation for moving the window?

There are several windows that overlap each other. This moves this window to an unused space. This is something to keep in mind when we do the larger UI update.

Is overlapping windows a problem?
Keep in mind the default minimum burrito overlay size is 800x600

Spatial.tscn

AsherGlick · 2024-01-24T10:09:37Z

Spatial.tscn

+margin_left = 777.0
+margin_top = 89.0
+margin_right = 1448.0
+margin_bottom = 587.0


Is overlapping windows a problem?
Keep in mind the default minimum burrito overlay size is 800x600

AsherGlick · 2024-01-24T11:30:37Z

Spatial.gd

+const user_protobin_by_map_id_dir: String = "user://protobin_by_map_id/"
+const user_marker_pack_dir: String = "user://marker_packs/"


This seems to describe more of what the variable is/contains/its value and not what its purpose is.

Suggested change

const user_protobin_by_map_id_dir: String = "user://protobin_by_map_id/"

const user_marker_pack_dir: String = "user://marker_packs/"

const processed_marker_packs_dir: String = "user://protobin_by_map_id/"

const downloaded_marker_packs_dir: String = "user://marker_packs/"

I dont think my suggustion is quite correct because "processed" is not fully descriptive of the fact that it also stored the autosaves. But the pendulum should probably swing back the other direction slightly.

I like downloaded_marker_packs_dir for the 2nd one.

How about something like active_data_dir. Any changes made with the editor would be saved in this folder and "active_data" conveys that it is mutable.

AsherGlick · 2024-01-24T11:33:33Z

Spatial.gd

-	self.unsaved_data_icon.visible = true
+func update_burrito_icon():
+	if self.unsaved_changes:
+		$Control/GlobalMenuButton/TextureRect.modulate = Color(.78, 0.3, 0.3, 1.0)


Lets save this color as a constant up at the top of the file.

Typically a value like this is called a "magic variable". Sometimes referred to by its type, such as "magic string" or "magic number" and it is not always clear why that value is there or what it does. If the line bcame

$Control/GlobalMenuButton/TextureRect.modulate = RED_TINT

or something like that, it becomes much more readable. Not sure "RED_TINT" is the right name.

I chose the color to be different enough to be noticed but I'm not set on it. For now I'm going to use the color constant "red".

AsherGlick · 2024-01-24T11:38:09Z

Spatial.gd

+		$Control/GlobalMenuButton/TextureRect.modulate = Color(.78, 0.3, 0.3, 1.0)
+		$Control/GlobalMenuButton/main_menu_toggle.hint_tooltip = "Unsaved Data"
+	else:
+		$Control/GlobalMenuButton/TextureRect.modulate = Color(1.0, 1.0, 1.0, 1.0)


You can use the default color constants here instead of 1,1,1,1

https://docs.godotengine.org/en/3.2/classes/class_color.html#constants

Understood. Using the white color constant. I also found that the alpha should have been lower than one to match the original value.

AsherGlick · 2024-01-24T11:47:00Z

Gizmo/PointEdit.gd

@@ -49,11 +50,11 @@ func update_point():
 			self.object_link.set_point_position(self.object_index, self.translation)
 			self.object_2d_link.points[self.object_index] = Vector2(self.translation.x, self.translation.z)
 		if point_type == "icon":
-			self.object_link.translation = self.translation
+			self.object_link.set_point_position(self.translation)


Gizmo is technically an external library so lets not modify it internally like this or else updating it for future versions will be harder.

Per our discussion, I have moved the functionality here into the main file. Gizmo should no longer care what type the point is put instead just emit its position when there is a change. This change did require me to make a class that can be used to contain all of the information gizmo was holding.

AsherGlick · 2024-01-26T04:18:57Z

PackDialog.gd

+var user_marker_pack_dir: String
+var user_protobin_by_map_id_dir: String


Same thing about naming these variables as the other comment about the variables that are named similarly.

AsherGlick · 2024-02-04T04:48:56Z

Spatial.gd

-	return data_out
+func save_current_map_data():
+	var packed_bytes = self.waypoint_data.to_bytes()
+	if packed_bytes.size() > 0:


Make and link issue to track this theoretical case of creating blank files. Because that is possible, maybe the correct logic here will be to delete files that are empty instead of not saving/overwriting them. But something a future pass can deal with.

AsherGlick · 2024-02-04T04:51:22Z

Spatial.gd

+	file.open(self.marker_file_path, file.WRITE)
+	file.store_buffer(packed_bytes)
+	self.unsaved_changes = false
+	update_burrito_icon()


How easy/likely is it for a programming mistake to lead to a desynced status and saved notification? EG: updating the variable without calling update_burrito_icon()

Very likely if the two lines are unknowingly coupled. I have written a setter function that can be called that would handle updating the icon.

…variables.

klingbolt · 2024-02-06T05:20:40Z

Spatial.tscn

@@ -85,7 +85,7 @@ __meta__ = {
 }

 [node name="TextureRect" type="TextureRect" parent="Control/GlobalMenuButton"]
-modulate = Color( 1, 1, 1, 0.439216 )
+modulate = Color( 1, 1, 1, 0.44 )


Rounded this to make it simpler to reset.

klingbolt · 2024-02-06T05:21:19Z

Spatial.tscn

@@ -185,7 +185,7 @@ __meta__ = {

 [node name="ImportPackDialog" type="FileDialog" parent="Control/Dialogs"]
 margin_left = 289.0
-margin_top = 36.0
+margin_top = 60.0


Moved this back but shrunk it down a bit so it didn't overlap the editor bar and burrito icon

AsherGlick

As we discussed offline, this might actually be 3 PRs

Removing the tight coupling of 3d and 2d routes in gizmo
Changing the source of truth of the node positions from 3d nodes to waypoint datums
Saving the waypoint data to file

Lets do a technical write up on number 2 too before continuing.

AsherGlick · 2024-04-20T23:43:29Z

Spatial.gd

@@ -389,12 +390,16 @@ func reset_3D_minimap_masks(category: Spatial):


 var waypoint_data: Waypoint.Waypoint = Waypoint.Waypoint.new()
-const user_protobin_by_map_id_dir: String = "user://protobin_by_map_id/"
-const user_marker_pack_dir: String = "user://marker_packs/"
+# We save the marker data in this directory where the files are have been split


are split or have been split

AsherGlick · 2024-04-20T23:44:08Z

Spatial.gd

-const user_protobin_by_map_id_dir: String = "user://protobin_by_map_id/"
-const user_marker_pack_dir: String = "user://marker_packs/"
+# We save the marker data in this directory where the files are have been split
+# by Map ID. All changes made by the editor are saved in these files.


Suggested change

# by Map ID. All changes made by the editor are saved in these files.

# by Map ID. All changes made by the editor are automatically saved in these files prior to export.

AsherGlick · 2024-04-21T00:03:06Z

Spatial.gd

@@ -690,30 +700,35 @@ func gen_adjustment_nodes():
 				continue
 			var new_gizmo = gizmo_scene.instance()
 			new_gizmo.translation = gizmo_position
-			new_gizmo.link_point("path", route, path2d, i)
-			new_gizmo.connect("selected", self, "on_gizmo_selected")
+			new_gizmo.connect("selected", self, "on_gizmo_selected", ["path", [route, path2d], i])


I think something is too clever here passing in an array of [route, path2d] or [icon] depending on the first argument being "path" or "icon".

AsherGlick · 2024-04-21T00:05:24Z

Spatial.gd

 	$Control/Dialogs/NodeEditorDialog/ScrollContainer/VBoxContainer/DeleteNode.disabled = false
 	# Only enable these buttons if the object selected is a point on the path not an icon
-	if object.point_type == "path":
+	if point_type == "path":


Can we have logic that does not rely on point_type branching in multiple places?

AsherGlick · 2024-04-21T00:07:30Z

Spatial.gd

-	update_burrito_icon()
+func on_point_updated(position: Vector3):
+	set_unsaved_changes(true)
+	if self.currently_selected_node.point_type == "icon":


Can we have logic that does not rely on point_type branching in multiple places?

AsherGlick · 2024-04-21T00:17:22Z

Spatial.gd

@@ -833,28 +854,28 @@ func _on_NodeEditorDialog_hide():

 func _on_DeleteNode_pressed():
 	if self.currently_selected_node.point_type == "icon":


Can we have logic that does not rely on point_type branching in multiple places?

AsherGlick · 2024-04-21T00:17:28Z

Spatial.gd


 		path.remove_point(index)
 		path2d.remove_point(index)
 	clear_adjustment_nodes()
 	gen_adjustment_nodes()
-	on_gizmo_deselected(self.currently_selected_node)
+	on_gizmo_deselected(self.currently_selected_node.gizmo)


 func _on_NewNodeAfter_pressed():
 	if self.currently_selected_node.point_type == "icon":


Can we have logic that does not rely on point_type branching in multiple places?

klingbolt · 2024-09-08T00:58:31Z

This PR is now obsolete.

klingbolt added 3 commits January 4, 2024 19:14

Adding saving functions

cd64268

Added spliting function

be8931c

Removing code that is not about saving current state

6bf21cb

AsherGlick requested changes Jan 20, 2024

View reviewed changes

Addressing code review

11e071c

AsherGlick requested changes Feb 4, 2024

View reviewed changes

klingbolt added 2 commits February 6, 2024 00:06

Addressing code review. Moving some functions out of Gizmo. Renaming …

1e65d69

…variables.

Renamed variables. Removed defunct function

e27380e

klingbolt commented Feb 6, 2024

View reviewed changes

AsherGlick requested changes Apr 21, 2024

View reviewed changes

klingbolt mentioned this pull request Jul 24, 2024

Save the current data to the map files #323

Merged

klingbolt closed this Sep 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save current state to split files #267

Save current state to split files #267

klingbolt commented Jan 20, 2024

AsherGlick Jan 20, 2024

klingbolt Jan 24, 2024

AsherGlick Feb 4, 2024

klingbolt Feb 6, 2024

AsherGlick Jan 20, 2024

klingbolt Jan 24, 2024

AsherGlick Jan 24, 2024

AsherGlick Jan 24, 2024

AsherGlick Jan 24, 2024

klingbolt Feb 6, 2024

AsherGlick Jan 24, 2024

klingbolt Feb 6, 2024

AsherGlick Jan 24, 2024

klingbolt Feb 6, 2024

AsherGlick Jan 24, 2024

klingbolt Feb 6, 2024

AsherGlick Jan 26, 2024

AsherGlick Feb 4, 2024

AsherGlick Feb 4, 2024

klingbolt Feb 6, 2024

klingbolt Feb 6, 2024

klingbolt Feb 6, 2024

AsherGlick left a comment

AsherGlick Apr 20, 2024

AsherGlick Apr 20, 2024

AsherGlick Apr 21, 2024

AsherGlick Apr 21, 2024

AsherGlick Apr 21, 2024

AsherGlick Apr 21, 2024

AsherGlick Apr 21, 2024

klingbolt commented Sep 8, 2024

		const user_protobin_by_map_id_dir: String = "user://protobin_by_map_id/"
		const user_marker_pack_dir: String = "user://marker_packs/"

		var user_marker_pack_dir: String
		var user_protobin_by_map_id_dir: String

	# by Map ID. All changes made by the editor are saved in these files.
	# by Map ID. All changes made by the editor are automatically saved in these files prior to export.

		@@ -833,28 +854,28 @@ func _on_NodeEditorDialog_hide():

		func _on_DeleteNode_pressed():
		if self.currently_selected_node.point_type == "icon":

Save current state to split files #267

Save current state to split files #267

Conversation

klingbolt commented Jan 20, 2024

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

AsherGlick 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

klingbolt commented Sep 8, 2024