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

Implement a way of rendering images without spritesheets (Fixes #1086) #1153

Merged
merged 1 commit into from Nov 17, 2018

Conversation

Projects
None yet
5 participants
@happenslol
Copy link
Contributor

happenslol commented Nov 15, 2018

A few things are still TODO here, I just wanted to open this up before I document all my changes.

As discussed in #1086, I've extended the DrawSprite pass to also take care of ImageRender structs, which allows the user to render full images without having to use configuration files or set up spritesheets. The same visibility sorting and filtering is used as for Sprites. This also means that we get sorting and batching for free, so multiple images rendered from the same texture will not cause a state switch as long as they are drawn right after each other.

To reflect that the DrawSprite pass now renders both sprites and full images, I've renamed it to DrawQuad, and changed all mentions of it. I'm not quite happy with the name and I'm not sure we even want to change it, so I'm open to suggestions here. I'm also not sure if SpriteVisibility should be renamed too.

As for tests, I've looked at the tests for sprite and they only make sure that sprite coordinates are parsed correctly. For the pass there were no tests, and I'm only passing in new data structures into old code anyways, so I wasn't really sure how to add new tests for my changes specifically.

Things that are still left to do after the code has been reviewed and revised:

  • Update the relevant sections in the book
  • Make sure all examples are up to date with the new naming
  • Update the changelog

Fixes #1086

@torkleyy
Copy link
Member

torkleyy left a comment

Thank you, I really like how you created this PR.
I have one concern though. I'm a bit unsure about renaming the pass for two reasons:

  • images can't be drawn without enabling sprites
  • searching for sprite does no longer get you to the correct pass

I think the problem we're experiencing here is that currently a pass has so much boilerplate that it's understandable to add things to an existing one rather than creating a new one. This boilerplate will be reduced in the future, but I don't know what to do about this now. I'm hoping somebody else (maybe @Rhuagh or @Moxinilian) could weigh in here.

@happenslol

This comment has been minimized.

Copy link
Contributor

happenslol commented Nov 16, 2018

Yeah I totally get that it makes both things (drawing sprites/drawing images) a little more complicated. I'm open to moving the DrawImage pass out, the passes are pretty much separated already inside of the DrawQuad pass, but as you said that would result in a lot more boilerplate. I'll wait on some more feedback here though before doing any further revisions.

@jojolepro

This comment has been minimized.

Copy link
Member

jojolepro commented Nov 16, 2018

I want to review this soon

@jojolepro
Copy link
Member

jojolepro left a comment

A good step in the right direction!

I left some notes for compability with the refactor changes which I want to start writing during the weekend.

Thanks a lot! :) 🎉 👍

Show resolved Hide resolved amethyst_renderer/src/img.rs Outdated
Show resolved Hide resolved amethyst_renderer/src/img.rs Outdated
Show resolved Hide resolved amethyst_renderer/src/pass/quad/interleaved.rs Outdated
Show resolved Hide resolved amethyst_renderer/src/pass/quad/interleaved.rs Outdated
Show resolved Hide resolved amethyst_renderer/src/pass/quad/interleaved.rs Outdated
Show resolved Hide resolved amethyst_renderer/src/pass/quad/interleaved.rs Outdated
Show resolved Hide resolved amethyst_renderer/src/pass/quad/interleaved.rs Outdated
Show resolved Hide resolved amethyst_renderer/src/pass/quad/interleaved.rs Outdated
@happenslol

This comment has been minimized.

Copy link
Contributor

happenslol commented Nov 16, 2018

@jojolepro Just implemented all of your review feedback. Flipped lives in the file for sprite right now, since that's what it's used for specifically. The Flipped change also made the pong examples a lot shorter, since you only need to create 1 paddle sprite_render component now.

I'm unsure if I should add a small bit about the Flipped component to the sprite_render_component tutorial, since there is no mention of how to flip sprites or textures now.

If you're fine with all these, I'll add a tutorial to the book and finish up the PR.

@jojolepro
Copy link
Member

jojolepro left a comment

Excellent! Thanks a lot! 👍

Show resolved Hide resolved docs/CHANGELOG.md
@DoumanAsh
Copy link
Contributor

DoumanAsh left a comment

There were not much to comment on, looks good.
Only few minor things

Show resolved Hide resolved amethyst_renderer/src/sprite.rs Outdated
Show resolved Hide resolved amethyst_renderer/src/pass/flat2d/interleaved.rs
Show resolved Hide resolved amethyst_renderer/src/pass/flat2d/interleaved.rs
@jojolepro

This comment has been minimized.

Copy link
Member

jojolepro commented Nov 17, 2018

Blocked per request of @Moxinilian until his review.

@Moxinilian
Copy link
Member

Moxinilian left a comment

I was about to complain about Flipped but the Both variant solved it. Thanks!
The changes are well made. If you don't want to address my comments, we may merge immediately.

Show resolved Hide resolved book/src/sprites/display_the_texture.md Outdated
Show resolved Hide resolved docs/AUTHORS.md Outdated

@happenslol happenslol force-pushed the happenslol:implement-simple-image-render branch 3 times, most recently from 60a888d to e67d499 Nov 17, 2018

Allow rendering 2D images without SpriteSheet
Add simple image example

Implement basic full image render support

Deduplicate code and improve naming in DrawSprite pass

Change naming for draw sprite to reflect new capabilities

Add support for visibility sorted images

Change naming for image module

Run rustfmt

Update changelog

Update authors

Revert unwanted autoformat changes

Implemented laminar

move spritesheet definition to next chapter

Branch to update winit and glutin.

Use `winit` "serde" feature for serialization.

Remove unused import for MacOS.

Implement review feedback

Update all examples and tutorials for Flipped component

Fix sprite_render move issues

Update changelog

Add PR link to changelog

Add book chapter for images

Fix meshes rendering in draw2dflat pass

Fix typos in new book chapter

Fix book example

Fix mutability issue in book example

Implement review feedback

Adjust authors file and texture display example

Add None variant for flipped

@happenslol happenslol force-pushed the happenslol:implement-simple-image-render branch from e67d499 to ff0713f Nov 17, 2018

@jojolepro

This comment has been minimized.

Copy link
Member

jojolepro commented Nov 17, 2018

Thanks a lot!

bors r+

bors bot added a commit that referenced this pull request Nov 17, 2018

Merge #1153
1153: Implement a way of rendering images without spritesheets (Fixes #1086) r=jojolepro a=happenslol

A few things are still TODO here, I just wanted to open this up before I document all my changes.

As discussed in #1086, I've extended the `DrawSprite` pass to also take care of `ImageRender` structs, which allows the user to render full images without having to use configuration files or set up spritesheets. The same visibility sorting and filtering is used as for `Sprite`s. This also means that we get sorting and batching for free, so multiple images rendered from the same texture will not cause a state switch as long as they are drawn right after each other.

To reflect that the `DrawSprite` pass now renders both sprites and full images, I've renamed it to `DrawQuad`, and changed all mentions of it. I'm not quite happy with the name and I'm not sure we even want to change it, so I'm open to suggestions here. I'm also not sure if `SpriteVisibility` should be renamed too.

As for tests, I've looked at the tests for sprite and they only make sure that sprite coordinates are parsed correctly. For the pass there were no tests, and I'm only passing in new data structures into old code anyways, so I wasn't really sure how to add new tests for my changes specifically.

Things that are still left to do after the code has been reviewed and revised:

* [x] Update the relevant sections in the book
* [x] Make sure all examples are up to date with the new naming
* [x] Update the changelog

Co-authored-by: Hilmar Wiegand <me@hwgnd.de>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Nov 17, 2018

@jojolepro

This comment has been minimized.

Copy link
Member

jojolepro commented Nov 17, 2018

bors r+

bors bot added a commit that referenced this pull request Nov 17, 2018

Merge #1153
1153: Implement a way of rendering images without spritesheets (Fixes #1086) r=jojolepro a=happenslol

A few things are still TODO here, I just wanted to open this up before I document all my changes.

As discussed in #1086, I've extended the `DrawSprite` pass to also take care of `ImageRender` structs, which allows the user to render full images without having to use configuration files or set up spritesheets. The same visibility sorting and filtering is used as for `Sprite`s. This also means that we get sorting and batching for free, so multiple images rendered from the same texture will not cause a state switch as long as they are drawn right after each other.

To reflect that the `DrawSprite` pass now renders both sprites and full images, I've renamed it to `DrawQuad`, and changed all mentions of it. I'm not quite happy with the name and I'm not sure we even want to change it, so I'm open to suggestions here. I'm also not sure if `SpriteVisibility` should be renamed too.

As for tests, I've looked at the tests for sprite and they only make sure that sprite coordinates are parsed correctly. For the pass there were no tests, and I'm only passing in new data structures into old code anyways, so I wasn't really sure how to add new tests for my changes specifically.

Things that are still left to do after the code has been reviewed and revised:

* [x] Update the relevant sections in the book
* [x] Make sure all examples are up to date with the new naming
* [x] Update the changelog

Fixes #1086 

Co-authored-by: Hilmar Wiegand <me@hwgnd.de>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Nov 17, 2018

@bors bors bot merged commit ff0713f into amethyst:master Nov 17, 2018

4 checks passed

bors Build succeeded
Details
concourse-ci/linux-book-tests Concourse CI build success
Details
concourse-ci/linux-tests Concourse CI build success
Details
concourse-ci/rustfmt Concourse CI build success
Details

@happenslol happenslol deleted the happenslol:implement-simple-image-render branch Nov 17, 2018

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