Skip to content

Conversation

@TheLimeGlass
Copy link
Contributor

Description

Improves Item Frames as Snow always wanted.

Sets the entity data pattern to

item[ ]frame(|1¦s) [(with|of) %-itemtype%] [[with rotation|rotated] %-rotation%]

for more proper support of ItemFrames while also registering ItemFrames as a classinfo for dedicated syntaxes.
Adds org.bukkit.Rotation and a utility effect for easily rotating an expression that returns a rotation and/or itemframes and rotation's self.

Examples:

# Spawning
spawn an item frame with lime stained glass rotated 225 degrees at target block
spawn an item frame of stone rotated 90 degrees in the direction of player from target block:
    rotate event-item frame by 90 degrees
    set item of event-item frame to lime stained glass named "example"

# EffRotate
rotate the event-item frame clockwise 2 times
rotate the event-item frame by 225 degrees

Target Minecraft Versions: any
Requirements: none
Related Issues: #414 (non-closing) and #2874

@TheLimeGlass TheLimeGlass requested a review from a team as a code owner March 26, 2025 00:12
@TheLimeGlass TheLimeGlass requested review from Romitou and sovdeeth and removed request for a team March 26, 2025 00:12
@TheLimeGlass TheLimeGlass changed the title Feature/itemframes Improve itemframes Mar 26, 2025
Copy link
Contributor

@Absolutionism Absolutionism left a comment

Choose a reason for hiding this comment

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

What do we think about making an expression to grab the rotation of an itemframe?
Also, not sure if possible, but combined with that expression, make a test structure within EffSecSpawn.sk spawning item frames with a rotation and ensuring the rotation is correct.

Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

T-te-test!

@sovdeeth
Copy link
Member

sovdeeth commented Mar 26, 2025

I'm concerned that this rotate effect will add a lot of ambiguity to the existing rotate effect for displays, vectors, and quaternions. I'd like to see the two integrated if possible. I'm also concerned about adding the Rotation class, as I think it will cause confusion with the existing ExprAngle expression.

Can we not treat item frames as any other rotatable object and just snap their alignment to 45 degree angles if the rotation would result in something like 60 degrees?

Co-authored-by: SirSmurfy2 <82696841+Absolutionism@users.noreply.github.com>
@sovdeeth sovdeeth added the feature Pull request adding a new feature. label Apr 8, 2025
@skriptlang-automation skriptlang-automation bot added the needs reviews A PR that needs additional reviews label May 15, 2025
@APickledWalrus APickledWalrus linked an issue Jun 4, 2025 that may be closed by this pull request
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

I'm concerned that this rotate effect will add a lot of ambiguity to the existing rotate effect for displays, vectors, and quaternions. I'd like to see the two integrated if possible. I'm also concerned about adding the Rotation class, as I think it will cause confusion with the existing ExprAngle expression.

Can we not treat item frames as any other rotatable object and just snap their alignment to 45 degree angles if the rotation would result in something like 60 degrees?

reminder to address/respond to these concerns

@skriptlang-automation skriptlang-automation bot removed the needs reviews A PR that needs additional reviews label Aug 26, 2025
@TheLimeGlass
Copy link
Contributor Author

I'm concerned that this rotate effect will add a lot of ambiguity to the existing rotate effect for displays, vectors, and quaternions. I'd like to see the two integrated if possible. I'm also concerned about adding the Rotation class, as I think it will cause confusion with the existing ExprAngle expression.
Can we not treat item frames as any other rotatable object and just snap their alignment to 45 degree angles if the rotation would result in something like 60 degrees?

reminder to address/respond to these concerns

They're different rotation objects

@sovdeeth
Copy link
Member

I'm concerned that this rotate effect will add a lot of ambiguity to the existing rotate effect for displays, vectors, and quaternions. I'd like to see the two integrated if possible. I'm also concerned about adding the Rotation class, as I think it will cause confusion with the existing ExprAngle expression.
Can we not treat item frames as any other rotatable object and just snap their alignment to 45 degree angles if the rotation would result in something like 60 degrees?

reminder to address/respond to these concerns

They're different rotation objects

I understand, but the point of my comment is that we shouldn't have two methods of rotating things, one that one works for x and one that only works for y. They should be unified so that all the rotation methods work with everything.

@TheLimeGlass
Copy link
Contributor Author

I understand, but the point of my comment is that we shouldn't have two methods of rotating things, one that one works for x and one that only works for y. They should be unified so that all the rotation methods work with everything.

Can you provide what you vision to have happen about the input of the rotation in the pattern since they're going to be different objects. They won't parse the same.

@TheLimeGlass TheLimeGlass requested a review from sovdeeth October 2, 2025 10:49
@sovdeeth
Copy link
Member

sovdeeth commented Oct 2, 2025

I understand, but the point of my comment is that we shouldn't have two methods of rotating things, one that one works for x and one that only works for y. They should be unified so that all the rotation methods work with everything.

Can you provide what you vision to have happen about the input of the rotation in the pattern since they're going to be different objects. They won't parse the same.

rotate {item frame} by %number% degrees which just snaps to the nearest 45 degree mark. Avoids the need for a rotation type that adds confusion about where it can be used (and possibly causes conflict with existing code that uses set {x} to 45 degrees).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Pull request adding a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SUGGESTION] Rotation of item held by clicked block

4 participants