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

Remember frames when selecting SpriteFrame frames #88413

Merged

Conversation

LeulMulugeta
Copy link
Contributor

@LeulMulugeta LeulMulugeta commented Feb 16, 2024

this will stop resting the options for "Select Frames" window on reopening an image or a subsequent opening of the same size image. this will be useful in the cases of :

  • working with a large sprite sheet containing multiple animations
  • and when each characters in a project have the same sprite sheet size.

@AThousandShips

This comment was marked as outdated.

@LeulMulugeta

This comment was marked as outdated.

@AThousandShips
Copy link
Member

AThousandShips commented Feb 16, 2024

This doesn't really fully resolve that proposal IMO, this won't work if the editor is restarted for example

Also what happens if you use a texture that's very different in size or shape?

See also (to make it clear this is an alternative to another PR):

@AThousandShips AThousandShips added this to the 4.x milestone Feb 16, 2024
@AThousandShips AThousandShips changed the title Dont reset select frames Remember frames when selecting SpriteFrame frames Feb 16, 2024
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Quickly testing this and it makes the editor essentially unusable when changing to a new texture, if you take a large texture first, where each frame is, lets say, 256x256, and then pick a texture that's 128x128, then you will get a single, 128x128 frame covering the whole image

If you then change back to the original texture it will give you a single 128x128 frame at the top left

This needs to detect other changes, and retain specific details, like the other PR did

@LeulMulugeta
Copy link
Contributor Author

ok.. i will make considerations for changes in texture size

@AThousandShips
Copy link
Member

As already seen in the other PR, and from my own experience implementing this, this isn't a trivial simple change to make, so I think you will have to rewrite this quite from the ground up

@LeulMulugeta LeulMulugeta requested review from a team as code owners February 17, 2024 18:54
@AThousandShips AThousandShips removed request for a team February 17, 2024 19:14
@LeulMulugeta
Copy link
Contributor Author

Sorry for the noise .. i was learning git :/

@AThousandShips
Copy link
Member

While these changes are an improvement they only work when the new texture is exactly the same size, which helps only a little bit, the proper solution would be to retain the mode, there's an edit mode used which selects if you are editing the number of sections, or the size of each, this could be used

This is still an improvement but I don't know if it alone is sufficient as a PR, but again the proper solution is complex

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks fine implementation-wise, but can't say how much useful is it in the current form and how well it would integrate with the full solution.

@akien-mga akien-mga requested a review from kleonc March 5, 2024 13:35
@akien-mga
Copy link
Member

Could you squash the commits, and amend the resulting commit to give it a clearer title (like the PR)? See PR workflow for instructions.

Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

As already mentioned by others this is a very simple implementation which doesn't solve the issue in general case. But it does improve a basic use case when subsequently creating separate animations from the same-layout spritesheets. For other use cases the usage should be as annoying as before. 🙃

So it's fine to merge, a better solution can be implemented later.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 5, 2024
@akien-mga akien-mga merged commit 0975e29 into godotengine:master Mar 5, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@LeulMulugeta LeulMulugeta deleted the dont_reset_select_frames branch March 6, 2024 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants