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

Macro ring #62

Open
wants to merge 50 commits into
base: master
Choose a base branch
from
Open

Macro ring #62

wants to merge 50 commits into from

Conversation

Master-Bw3
Copy link
Contributor

No description provided.

Master-Bw3 and others added 17 commits August 29, 2024 20:55
# Conflicts:
#	src/client/java/dev/enjarai/trickster/screen/SpellPartWidget.java
#	src/main/java/dev/enjarai/trickster/spell/fragment/ListFragment.java
#	src/main/java/dev/enjarai/trickster/spell/trick/func/ClosureTrick.java
# Conflicts:
#	src/client/java/dev/enjarai/trickster/screen/SpellPartWidget.java
#	src/main/java/dev/enjarai/trickster/spell/fragment/ListFragment.java
#	src/main/java/dev/enjarai/trickster/spell/trick/func/ClosureTrick.java
@Master-Bw3 Master-Bw3 marked this pull request as draft October 3, 2024 00:01
@StellarWitch7
Copy link
Collaborator

Oh this'll be fun to review...

@StellarWitch7 StellarWitch7 self-assigned this Oct 3, 2024
@StellarWitch7 StellarWitch7 added the enhancement New feature or request label Oct 3, 2024
@StellarWitch7
Copy link
Collaborator

Regarding the following trick:
image

A version of this was already created on master previously, as documented below:
image

@Master-Bw3, @enjarai, and @Vlue, I'd like to keep only one of these. I personally prefer the one that accepts an address, as it's more versatile. But I'm not too set on the naming or pattern. Thoughts? (Pinging cotton as she was particularly interested in being able to get a list.)

@StellarWitch7
Copy link
Collaborator

I've made some changes that I'll push in a moment. Otherwise, the only issue I notice is that the macro map read/write tricks don't work if the ring is in the second slot. Also I'm sure enjarai has something to say about them being in the items category...

@StellarWitch7
Copy link
Collaborator

StellarWitch7 commented Oct 3, 2024

Oh last thing, I promise. It'd be nice if the trickster.spell.fragment.Map package didn't exist, and especially not capitalized in such a way. Perhaps Hamt would enjoy another home?

@StellarWitch7
Copy link
Collaborator

Anything I haven't covered, I've either touched in my commit, or it looks fine enough! Very nice PR, thank you.

@StellarWitch7
Copy link
Collaborator

The GitHub Actions build keeps failing, but even on a fresh clone of the repo it works for me... strange.

@StellarWitch7
Copy link
Collaborator

It's somehow my fault, but I don't know why...

@StellarWitch7
Copy link
Collaborator

StellarWitch7 commented Oct 3, 2024

Oh, the datagen fails on my machine. That's funny. Next commit should fix it.

Copy link
Collaborator

@StellarWitch7 StellarWitch7 left a comment

Choose a reason for hiding this comment

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

Apart from the missing texture (and I think a model, too?), I think this is good to merge now!

@StellarWitch7 StellarWitch7 marked this pull request as ready for review October 12, 2024 02:27
@enjarai
Copy link
Owner

enjarai commented Oct 18, 2024

Texture added

@StellarWitch7
Copy link
Collaborator

StellarWitch7 commented Oct 30, 2024

Very important: worlds created with the latest commit, and in which a fragment is stored within an item, cannot be loaded once saved. This is a critical issue that must be addressed as soon as possible. The issue is likely in EndecTomfoolery or FragmentComponent and stems from the attempt to merge SpellComponent and MapComponent. Fixed.

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.

3 participants