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

Wave Function Collapse Environments #371

Merged
merged 229 commits into from
Oct 11, 2023

Conversation

jysdoran
Copy link
Contributor

@jysdoran jysdoran commented Jun 29, 2023

Description

This PR is still work in progress.

This PR implements a new feature for Minigrid that allows procedural generation of environments using the Wave Function Collapse algorithm (adapted from this python implementation)

I tried to port the relevant git history of both external repository sources but it has made things a bit messy. Pull requests usually have a squash on merge function so it probably won't affect the main branch but I can go in and try to squash some commits together before then too. Most of the relevant adaptation changes are by me at the end of the commit history.

TODO:

  • Convert tests
  • Add more presets
  • Seed environment tests to guarantee pass

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Screenshots

These are some example environments generated by the SimpleMaze preset:

image
image

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Contributor

@francelico francelico left a comment

Choose a reason for hiding this comment

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

Looks great overall, left some minor comments on the PR.

minigrid/__init__.py Outdated Show resolved Hide resolved
minigrid/__init__.py Outdated Show resolved Hide resolved
minigrid/envs/__init__.py Outdated Show resolved Hide resolved
minigrid/envs/wfc/wfclogic/__init__.py Outdated Show resolved Hide resolved
minigrid/envs/wfc/wfcroom.py Outdated Show resolved Hide resolved
minigrid/envs/wfc/wfcroom.py Outdated Show resolved Hide resolved
minigrid/envs/wfc/config.py Show resolved Hide resolved
@jysdoran
Copy link
Contributor Author

jysdoran commented Jul 8, 2023

You will need to update the pip install

RUN pip install .[testing] --no-cache-dir

to pip install .[wfc, testing]

@pseudo-rnd-thoughts Thanks for the tip, I have updated the dockerfile but it seems to be having trouble identifying the wfc extra. I have tested a local installation with pip install -e .[wfc] which seems work fine with just the toml change. I also noticed these requirements.txt and test_requirements.txt files - would I need one for wfc?

@pseudo-rnd-thoughts pseudo-rnd-thoughts marked this pull request as ready for review July 9, 2023 22:12
Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

Could you replace in pre-commit-config flake8 # - --ignore with - --ignore=E203, W503
There is an issue that Literal doesn't exist in typing, rather typing-extensions within py 3.7, but we can actually drop py 3.7 now with python no longer supporting it

@pseudo-rnd-thoughts
Copy link
Member

@jysdoran It looks like there is an issue with the test_wrappers.py, could you see if you can solve it otherwise, could you remove the wfc-environment from the tests

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

Could you check that the wfclogic functions are all actually used
Also we are missing the copyright docstring at the top of each wfclogic file

minigrid/__init__.py Outdated Show resolved Hide resolved
minigrid/envs/__init__.py Outdated Show resolved Hide resolved
minigrid/envs/wfc/config.py Outdated Show resolved Hide resolved
minigrid/envs/wfc/graphtransforms.py Outdated Show resolved Hide resolved
minigrid/envs/wfc/graphtransforms.py Outdated Show resolved Hide resolved
tests/test_wfc/conftest.py Outdated Show resolved Hide resolved
tests/test_wfc/test_wfc_tiles.py Outdated Show resolved Hide resolved
tests/test_wfc/test_wfc_solver.py Outdated Show resolved Hide resolved
tests/test_wfc/test_wfc_solver.py Outdated Show resolved Hide resolved
tests/test_wfc/test_wfc_solver.py Outdated Show resolved Hide resolved
@pseudo-rnd-thoughts
Copy link
Member

@jysdoran Hey, how is this going?
It seems that the CI failed because RuntimeError: Could not generate a valid pattern within 100 attempts.
Could we see if we can resolve this issue along with the two requested changes

Then I think we are close to merging the PR

@jysdoran
Copy link
Contributor Author

@pseudo-rnd-thoughts, I think everything is passing and should be ready to go. I refactored it such that the environments are registered but the test cases are skipped. I had to add a dummy class to the wfc/__init__.py, let me know if you think there's a better solution because I'm not sure.

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

Hey, sorry for being so slow about approving this. Yes, I believe you have addressed all of questions

@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit e726259 into Farama-Foundation:master Oct 11, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants