Added logic to traverse the dimensions directory tree #251

Podshot · 2023-07-18T03:33:12Z

This code changes now traverses the dimensions directory tree and searches for the following directories to exist to determine if the parent directory is a dimension: data, entities, poi and region

Notes:

This logic can also be migrated to anvil_world/format.py here but I put it into the anvil_forge_world.py file for now since it's Forge specific. Feel free to let me know if you want me to migrate it

Resolves Amulet-Team/Amulet-Map-Editor#922

CLAassistant · 2023-07-18T03:33:19Z

All committers have signed the CLA.

gentlegiantJGC · 2023-07-18T12:26:33Z

amulet/level/formats/anvil_forge_world.py

@@ -33,5 +39,15 @@ def game_version_string(self) -> str:
        except Exception:
            return f"Java Forge Unknown Version"

+    def _register_modded_dimensions(self):
+        for path, directories, files in os.walk(os.path.join(self.path, "dimensions")):
+            if set(("data", "entities", "poi", "region")).issubset(set(directories)):


This can be simplified to the following

if {"data", "entities", "poi", "region"}.issubset(directories):

gentlegiantJGC · 2023-07-18T12:47:49Z

amulet/level/formats/anvil_forge_world.py

@@ -33,5 +39,15 @@ def game_version_string(self) -> str:
        except Exception:
            return f"Java Forge Unknown Version"

+    def _register_modded_dimensions(self):
+        for path, directories, files in os.walk(os.path.join(self.path, "dimensions")):


files isn't used so this can be an _

gentlegiantJGC · 2023-07-18T12:49:35Z

amulet/level/formats/anvil_forge_world.py

+            if set(("data", "entities", "poi", "region")).issubset(set(directories)):
+                dimension_path_split = path.split(os.sep)
+                dimensions_directory_index = dimension_path_split.index("dimensions")
+                dimension_name = f"{dimension_path_split[dimensions_directory_index + 1]}:{'/'.join(dimension_path_split[dimensions_directory_index + 2:])}"


You should probably add a check if we are at least two directories deep otherwise this will error

gentlegiantJGC · 2023-07-18T12:51:34Z

amulet/level/formats/anvil_forge_world.py

+        for path, directories, files in os.walk(os.path.join(self.path, "dimensions")):
+            if set(("data", "entities", "poi", "region")).issubset(set(directories)):
+                dimension_path_split = path.split(os.sep)
+                dimensions_directory_index = dimension_path_split.index("dimensions")


You compute the relpath further down. The indexes will be constant in that output.

gentlegiantJGC · 2023-07-18T12:54:15Z

If only the region directory is required this can be compacted down to a glob.
I can make these changes if you are busy.

gentlegiantJGC · 2023-07-18T13:09:38Z

I haven't tested this but this is how I would implement it

    def _register_modded_dimensions(self):
        for region_path in glob.glob(
            os.path.join(glob.escape(self.path), "dimensions", "*", "**", "region"), 
            recursive=True
        ):
            dim_path = os.path.dirname(region_path)
            if not {"data", "entities", "poi", "region"}.issubset(filter(os.path.isdir, os.listdir(dim_path))):
                continue
            rel_dim_path = os.path.relpath(dim_path, self.path)
            _, dimension, *base_name = rel_dim_path.split(os.sep)

            dimension_name = f"{dimension}:{'/'.join(base_name)}"
            self._register_dimension(
                os.path.dirname(rel_dim_path), dimension_name
            )

Podshot · 2023-07-18T20:59:04Z

Yeah I can test that glob code and see if it needs any additional changes but from a quick look it looks like it will work. I'll amend this later tonight

…antJGC

…antJGC with tweaks for it to properly check if the required paths are directories

amulet/level/formats/anvil_forge_world.py

…the check in-case /region is a file and not a directory

gentlegiantJGC · 2023-07-23T15:20:34Z

amulet/level/formats/anvil_forge_world.py

+                for basename in os.listdir(dim_path)
+                if os.path.isdir(os.path.join(dim_path, basename))
+            )
+            if not {"region"}.issubset(child_dir_names):


Since you only check one directory it should be.
The os.path.dirname in the last line was also wrong.

def _register_modded_dimensions(self): for region_path in glob.glob( os.path.join(glob.escape(self.path), "dimensions", "*", "**", "region"), recursive=True, ): if not os.path.isdir(region_path): continue dim_path = os.path.dirname(region_path) rel_dim_path = os.path.relpath(dim_path, self.path) _, namespace, *base_name = rel_dim_path.split(os.sep) dimension_name = f"{namespace}:{'/'.join(base_name)}" self._register_dimension(rel_dim_path, dimension_name)

I'll add this in tomorrow when I get the chance! I wanted to keep the directory check in and decided to keep the set approach in-case other directories would need to be added in the future but I think this is good and we can just do that change if/when it would be needed

`"*", "*", "**"` is required to match at least two directories deep. dirname should not be on the last line. dirname there would give a different directory.

gentlegiantJGC · 2023-08-16T09:11:40Z

Delved into the code to work out why the tests were breaking.
This code shouldn't be in the constructor. There is a method to reload world data with the same instance and it should be in there.
I will make these changes.

Moved the modded dimension finding code into the vanilla class. The tests were breaking because the code was implemented in the constructor. This code should be in the level reloading method.

Podshot · 2023-08-16T11:33:06Z

Delved into the code to work out why the tests were breaking.

This code shouldn't be in the constructor. There is a method to reload world data with the same instance and it should be in there.

I will make these changes.

Ah gotcha, is there a particular reason why the tests just started to fail? They were passing fine until the commit where we only check the region directory.

I also think the logic should still be contained in anvil_forge_world.py since it's not vanilla behavior for the world to have more (or more rather, a non-static/dynamic number of dimensions) other than the Nether and End

gentlegiantJGC · 2023-08-16T13:43:11Z

Your code caused a modded dimension to get registered twice (once in your code and once in the vanilla code) which triggered another issue. I don't think I finished implementing the reloading code so there are a number of issues.

It shouldn't do any harm in the vanilla class and is less likely to get broken when I refactor those classes.

I want to refactor the level wrappers so that the constructors don't actually load the world. They just initialise some default values.
Classmethods will do the actual loading.
The reason is because the same class is used to load an existing level and to create a level.

Podshot added 2 commits July 18, 2023 03:26

Added logic to traverse the dimensions directory tree

d985e11

Removed old logic that's no longer needed

16d59d3

Podshot requested a review from gentlegiantJGC July 18, 2023 03:33

Podshot self-assigned this Jul 18, 2023

gentlegiantJGC requested changes Jul 18, 2023

View reviewed changes

Reworked region finding logic to use the glob approach from @gentlegi…

654d31a

…antJGC with tweaks for it to properly check if the required paths are directories

Podshot requested a review from gentlegiantJGC July 19, 2023 03:18

gentlegiantJGC requested changes Jul 19, 2023

View reviewed changes

amulet/level/formats/anvil_forge_world.py Outdated Show resolved Hide resolved

Switched from map/filter functions to a list comprehension

831ee57

Podshot requested a review from gentlegiantJGC July 20, 2023 03:47

Removed unneeded directories in child directory names check, keeping …

81f6678

…the check in-case /region is a file and not a directory

Podshot mentioned this pull request Jul 23, 2023

Amulet CAN'T open some Modded Dimensions Amulet-Team/Amulet-Map-Editor#922

Closed

gentlegiantJGC requested changes Jul 23, 2023

View reviewed changes

Podshot and others added 3 commits July 27, 2023 20:24

Change to just check the region directory

d21d493

Fixed two bugs

5f24f82

`"*", "*", "**"` is required to match at least two directories deep. dirname should not be on the last line. dirname there would give a different directory.

Reformatted

f4c91da

Replaced old dimension loading

5eb10c7

Moved the modded dimension finding code into the vanilla class. The tests were breaking because the code was implemented in the constructor. This code should be in the level reloading method.

gentlegiantJGC approved these changes Aug 16, 2023

View reviewed changes

gentlegiantJGC merged commit dafef97 into main Aug 16, 2023
10 checks passed

gentlegiantJGC deleted the impl-more-modded-dimensions branch August 16, 2023 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added logic to traverse the dimensions directory tree #251

Added logic to traverse the dimensions directory tree #251

Podshot commented Jul 18, 2023

CLAassistant commented Jul 18, 2023 •

edited

Loading

gentlegiantJGC Jul 18, 2023

gentlegiantJGC Jul 18, 2023

gentlegiantJGC Jul 18, 2023

gentlegiantJGC Jul 18, 2023

gentlegiantJGC commented Jul 18, 2023

gentlegiantJGC commented Jul 18, 2023

Podshot commented Jul 18, 2023 •

edited

Loading

gentlegiantJGC Jul 23, 2023

Podshot Jul 25, 2023 •

edited

Loading

gentlegiantJGC commented Aug 16, 2023

Podshot commented Aug 16, 2023 •

edited

Loading

gentlegiantJGC commented Aug 16, 2023

Added logic to traverse the dimensions directory tree #251

Added logic to traverse the dimensions directory tree #251

Conversation

Podshot commented Jul 18, 2023

CLAassistant commented Jul 18, 2023 • edited Loading

gentlegiantJGC Jul 18, 2023

Choose a reason for hiding this comment

gentlegiantJGC Jul 18, 2023

Choose a reason for hiding this comment

gentlegiantJGC Jul 18, 2023

Choose a reason for hiding this comment

gentlegiantJGC Jul 18, 2023

Choose a reason for hiding this comment

gentlegiantJGC commented Jul 18, 2023

gentlegiantJGC commented Jul 18, 2023

Podshot commented Jul 18, 2023 • edited Loading

gentlegiantJGC Jul 23, 2023

Choose a reason for hiding this comment

Podshot Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

gentlegiantJGC commented Aug 16, 2023

Podshot commented Aug 16, 2023 • edited Loading

gentlegiantJGC commented Aug 16, 2023

CLAassistant commented Jul 18, 2023 •

edited

Loading

Podshot commented Jul 18, 2023 •

edited

Loading

Podshot Jul 25, 2023 •

edited

Loading

Podshot commented Aug 16, 2023 •

edited

Loading