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

Allow region size changes #447

Merged
merged 10 commits into from
Sep 24, 2024

Conversation

Dekker3D
Copy link
Contributor

@Dekker3D Dekker3D commented Aug 4, 2024

Update by TokisanGames

Fixes #77
Fixes #487

  • Enable changing region sizes, reslicing data (by Dekker3D)
  • Change do_for_regions to work with Callables for all language support
  • Region size is set automatically from the first loaded file
  • Add a UI for changing region size
  • Bug: occasionally reports regions are missing upon saving. They are null. See Allow region size changes #447 (comment)
  • Bug: Some sequences produce regions that are not blank but have data copied from other regions See Allow region size changes #447 (comment)
  • Bug: Heights and AABBs are messed up after slicing.
  • Bug: Occasionally prints a get_dependencies error See Allow region size changes #447 (comment)
  • Bug: Cannot go from 2 adjacent squares to the next size higher or that square disappears. Must have kitty-corner squares to fill in the blanks.
  • Foliage needs to be sliced and moved as well. Currently it is deleted.
  • Bug: Cannot import upgrade files then change region size or terrain disappears. Must save first, then reload the scene before region size can be changed.

Original PR

This should fix #77, and I haven't been able to spot any remaining issues regarding different region sizes. Regions are resized when you change the size, so if you have smaller regions that don't fully fill up a larger region when you change to a larger size, the other parts of that region will get default values.

The code for resizing is a little ugly (much uglier when sizing up, rather than down) and probably not very well optimized, but it works, and people are unlikely to change region sizes so often that it's a show-stopper.

@TokisanGames TokisanGames changed the title Allow region size changes WIP: Allow region size changes Aug 4, 2024
@TokisanGames TokisanGames added the enhancement New feature or request label Aug 4, 2024
@TokisanGames
Copy link
Owner

In my new PR, Terrain3DStorage has been renamed to Terrain3DData and changed from a savable resource to an Object. It won't save any data itself. All of the saved data are in Terrain3DRegions or the scene file. I'm moving the (global) region_size setting to Terrain3D.

This PR reslices when changing the setting. Fine with a few regions, but dangerous with 256-2048 regions.

What about undo/redo? If it's an inspector setting, Godot will undo/redo that setting which will reslice each time. Also not a great user experience.

The regions track their own region size based on the size of the first map loaded and validate subsequent loads against it. What if you're moving region files from one project to another? You need to set the right global region size setting before loading them or you'll get weird/broken results. Maybe when loading regions, it automatically sets global region size based on the first region and complains about any regions that are different. Once loaded the user can manually reslice if desired.

I'm thinking about rather than giving them an enum to choose and reslicing all data each time they change it, instead:

  • Region_size is read-only in the inspector
  • Add a menu option where they can resize regions
  • Add an API function to resize regions, called by the above. Then users will expect a process. We won't need an undo, they can just reslice.

In my PR all changes are done in memory (add/remove regions, sculpting, painting) and are not saved to disk until the scene is saved. Reslicing should work the same. Looks like this PR also does that.

Looks like this is a minor addition to what is already here, but my new region structure will require more updating.

@TokisanGames
Copy link
Owner

My PRs are merged. Please rebase and update this so we can merge it in.

@TokisanGames TokisanGames self-assigned this Sep 12, 2024
src/terrain_3d_data.h Outdated Show resolved Hide resolved

for (Ref<Terrain3DRegion> r : old_regions) {
remove_region(r, false);
}
Copy link
Owner

@TokisanGames TokisanGames Sep 12, 2024

Choose a reason for hiding this comment

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

remove_region doesn't remove from _regions, it only marks those regions as deleted.
When add_region comes in, it is adding regions in a populated dictionary, but the indexed keys are two different scales.
Saving after this operation reports missing regions because _regions has been corrupted.
Rather than remove_region, _regions should just be erased, and new regions added. The region map will be rebuilt in update_maps.

Those old regions will be freed if unreferenced, unless stored by the undo/redo system.

for (int y = index_bounds.position.y; y < index_bounds.get_end().y; y++) {
current_region_loc.y = y;
for (int x = index_bounds.position.x; x < index_bounds.get_end().x; x++) {
current_region_loc.x = x;
Copy link
Owner

Choose a reason for hiding this comment

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

All of this receives integer points in p_bounds, then converts to float to get the region location. Then it iterates through the included region locations. There's no reason or benefit to limit input to integers, or even calculate here. Instead, it can receive real world global coordinates and use get_region_location() to convert to location, including descaling.

Also below will crash on line 84 (region->get...) if region is null and p_do_empty_regions is true.

Copy link
Contributor Author

@Dekker3D Dekker3D Sep 13, 2024

Choose a reason for hiding this comment

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

Ah, I didn't test at all with p_do_empty_regions set to true. It's just a thing that LayerProcGen supports and it seemed sensible to do the same here. Like when you want to fill an area with a default colour, if no terrain exists there.

The main reason this version is limited to integers, is because it'll slow everything down by a lot if you need to consider interpolation. For one, interpolation can require you to use more than one region at the same time. I haven't really come up with a clean way to do that yet. (Later edit: I was mostly thinking of the per-pixel version of this function, but that will require the per-region version, and the int based per-pixel function would work best with an int-based per-region function)

Copy link
Owner

@TokisanGames TokisanGames Sep 14, 2024

Choose a reason for hiding this comment

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

I'm going to drop do_empty_regions. You're calling the region and have nothing to call if it's null. We can add more functionality later based on need. Perhaps it can add empty regions, then call them, but right now there's no use for it.

@TokisanGames
Copy link
Owner

I think dd729a4 is much more versatile, allowing us to provide copy/paste functionality to the editor in the future, and exposing do_for_regions to all languages. Now we can do this in gdscript.

func _ready() -> void:
	print("Readying do for regions")
	data.do_for_regions(Rect2i(0,-3072, 2048, 2048), 
		func(region:Terrain3DRegion, sarea:Rect2i, darea:Rect2i): 
			print("GDScript: ", region, sarea, darea)
	)

GDScript: [Wrapped:0][P: (0, 1024), S: (1024, 1024)][P: (0, 0), S: (1024, 1024)]
GDScript: [Wrapped:0][P: (1024, 1024), S: (1024, 1024)][P: (0, 0), S: (1024, 1024)]

@TokisanGames
Copy link
Owner

TokisanGames commented Sep 14, 2024

This is working well with Callable and reslicing is very fast.

However, there are four outstanding bugs that I haven't tracked down yet. They are present in your original and my current. My note about making a new _regions database might be accurate and help these. This is what I'm working on next.

  • 1. Sometimes when increasing/decreasing/adding/removing regions then saving, it reports that several regions cannot be found. In the code it means _regions[location].is_null() == true. These locations are ones not visible/present. I haven't found a reliable sequence that triggers this, but it only occurs on saving. We could just remove the null entries, but I'd like to know why. See Allow region size changes #447 (comment)

  • 2. Starting at the 3x1024 demo data, change to 2048. Then 512. Delete four regions in a corner (say +x, +z). Change to 2048. One region already has data.

    • Alternatively start at 1024, change to 512, change to 1024, change to 2048. Here the same corner region looks like above, but 4 other sections have extra data.

{6BDBB2CD-20F5-4B38-A946-AA053C8AE5C5}

  • 3. Sometimes when saving it prints "Writing region (0, 0) to res://demo/data/terrain3d_00_00.res", then prints after all saving:
ERROR: Method/function failed.
at: get_dependencies (scene/resources/resource_format_text.cpp:1771)

This is the engine source
https://github.com/godotengine/godot/blob/4.2/scene/resources/resource_format_text.cpp#L1771

It might be attempting to get_path() on an invalid region or something. I'm not sure. Might have to debug the engine for that one.

Update: I get this in 4.2.2 when I've attached a built in script to Terrain3D for testing, and ctrl+alt+save the script. Above when I've gotten it has been from ctrl+s to save the scene. This error is likely an engine bug with built in scripts, so I'll ignore it for now.

  • 4. It needs to load the region size from the files. If the size saved in the scene is out of sync from the files, it will reslice them, probably adding/removing regions and padding or cropping data.

@TokisanGames
Copy link
Owner

TokisanGames commented Sep 16, 2024

Exposing copy_paste allows this to work in gdscript. It currently copies the contents of region maps from range 0 to region_size. In the future this should be expanded to work with a rect2i in global coordinates, then loop through regions within the area

	data.copy_paste(data.get_region(Vector2i(0,-2)), Rect2i(0,0,512,512),  data.get_region(Vector2i(0,0)), Vector2i(0,0))
	data.force_update_maps()

@TokisanGames
Copy link
Owner

Alright, I found a sequence that causes No region found errors in both the original PR and my changes.

  1. Load the default demo data, 3 x 1024.
    region_locations: [(0, -1), (0, 0), (0, -2)]
    _regions: { (0, -1): [Wrapped:0], (0, 0): [Wrapped:0], (0, -2): [Wrapped:0] }

  2. Convert to 2048
    region_locations: [(0, -1), (0, 0)]
    _regions: { (0, -1): [Wrapped:0], (0, 0): [Wrapped:0], (0, -2): [Wrapped:0], (1, -2): , (1, -1): , (1, 0): , (0, 1): , (1, 1): }

  3. Save
    Reports

ERROR: Terrain3DData#9529:save_region: No region found at: (1, -2)
ERROR: Terrain3DData#9529:save_region: No region found at: (1, -1)
ERROR: Terrain3DData#9529:save_region: No region found at: (1, 0)
ERROR: Terrain3DData#9529:save_region: No region found at: (0, 1)
ERROR: Terrain3DData#9529:save_region: No region found at: (1, 1)
   at: push_error (core/variant/variant_utility.cpp:1091)

region_locations: [(0, -1), (0, 0)]
_regions: { (0, -1): [Wrapped:0], (0, 0): [Wrapped:0], (1, -2): , (1, -1): , (1, 0): , (0, 1): , (1, 1): }

Something is inserting ghost regions into _regions.

@TokisanGames
Copy link
Owner

TokisanGames commented Sep 17, 2024

Attempting to put this in Out of the Ashes, I realized this doesn't reslice any of the foliage data. It just erases it. Also the AABBs need to be recalculated. I have updated the top post with a tracker of tasks to do.

@TokisanGames TokisanGames force-pushed the Attempted-region-size-change branch 2 times, most recently from fb8c692 to b3ca706 Compare September 22, 2024 08:36
@TokisanGames
Copy link
Owner

Updates:

  • Identified the get_dependencies error as a godot bug unrelated to Terrain3D.
  • Fixed a bug all throughout Terrain3DData that created null regions, which are detected on save
  • Fixed some sequences creating new region space with duplicate data from random regions instead of blank/flat
  • Fixed issues expanding a single or 2 adjacent regions to a larger size (disappeared), and having it properly fill out to the edges of the region it is in instead of moving to another, (-1, -1 to 0, 0 moved to 0, 0 to 1, 1).
  • It's now working with vertex_scaling

I'm working on moving foliage properly

@TokisanGames TokisanGames force-pushed the Attempted-region-size-change branch 2 times, most recently from 7103aa8 to 913319e Compare September 23, 2024 13:49
@TokisanGames
Copy link
Owner

This PR is ready for review and testing.

There's just one last bug I'm tracking down: region sizes can't be changed right after upgrading. You must upgrade to the new storage, save, and reload the scene before region sizes can be changed.

@TokisanGames TokisanGames marked this pull request as ready for review September 23, 2024 15:18
@TokisanGames TokisanGames changed the title WIP: Allow region size changes Allow region size changes Sep 23, 2024
empty dir, update color mipmaps after rs, warn and stop if upgrading w/
full directory, Update filesystem only after saving directory
@TokisanGames TokisanGames merged commit 40cf9aa into TokisanGames:main Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra regions after setting up navigation Support more region sizes
2 participants