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

Replace Bloom filter with compressed dictionary #8

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ZetaTwo
Copy link

@ZetaTwo ZetaTwo commented Feb 7, 2022

  • Remove bloom filter
  • Add compressed dictionary lookup
  • Replace a bunch of magic values with constants
  • Fix a bug where guesses was [5][6] when they should be [6][6] since they are used as null-terminated strings

@ZetaTwo ZetaTwo mentioned this pull request Feb 7, 2022
@nezza
Copy link
Contributor

nezza commented Feb 8, 2022

Awesome work :) Where did you get the wordlist from? I wonder whether there are any copyright implications :/

@leo60228
Copy link

leo60228 commented Feb 8, 2022

The Wordle source code, presumably, which itself came from Collins Scrabble Words.

@leo60228
Copy link

leo60228 commented Feb 8, 2022

I'm not a lawyer so take this with a massive grain of salt, but I think this would infringe on database rights in jurisdictions with them. The US doesn't have those, but I think it's ambiguous whether copyright applies. There's a notable Supreme Court decision that phonebooks are not eligible for copyright, but that's arguably different from a wordlist.

@ZetaTwo
Copy link
Author

ZetaTwo commented Feb 8, 2022

Thanks! If it makes things easier, I can just remove the wordlist and let whoever builds the ROM supply their own list that they happen to "find" on the internet. :)

@nezza
Copy link
Contributor

nezza commented Feb 8, 2022

Hmm there must be some open-source dictionaries we can use

@ZetaTwo
Copy link
Author

ZetaTwo commented Feb 8, 2022

Yes but then it's not really Wordle anymore :D

@ZetaTwo
Copy link
Author

ZetaTwo commented Feb 8, 2022

But yeah, the words are definitely from Collins: https://en.wikipedia.org/wiki/Collins_Scrabble_Words 12972 is way to specific to be an accident.

@ZetaTwo
Copy link
Author

ZetaTwo commented Feb 8, 2022

Removed any copyrighted materials from the PR and integrated the C file generation into the Makefile. This should be completely safe to merge now.

Copy link

@M4R7iNP M4R7iNP left a comment

Choose a reason for hiding this comment

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

I've tested it and it works 💯

Makefile Outdated Show resolved Hide resolved
@@ -15,11 +15,14 @@ LCC = $(GBDK_HOME)bin/lcc
PROJECTNAME = WORDLE

BINS = $(PROJECTNAME).gb
CSOURCES := $(wildcard *.c)
CSOURCES := main.c word-db.c words.c
Copy link

Choose a reason for hiding this comment

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

Suggested change
CSOURCES := main.c word-db.c words.c
CSOURCES := $(wildcard *.c)

Copy link
Author

Choose a reason for hiding this comment

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

This breaks if you compile from a fresh checkout since words.c does not exist and needs to be generated from the python script so it needs to be specified manually but you can't have *.c and words.c because that will include it twice so in that case the CSOURCES needs to be "all files *.c except words.c" + "words.c" explicitly.

@@ -0,0 +1,16 @@
#include "word-db.h"
Copy link

Choose a reason for hiding this comment

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

Is this file unused?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the same template is part of the Python generate script but I wanted to make it clear what the file will look like.

Ooops, that should not have been commited.

Co-authored-by: Martin Pedersen <[email protected]>
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.

4 participants