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

Add ggez example #161

Merged
merged 25 commits into from May 10, 2022
Merged

Add ggez example #161

merged 25 commits into from May 10, 2022

Conversation

PieKing1215
Copy link
Member

Adds a rendering demo for ggez.
Supports multiple layers, multiple tilesets, and parallax.
Also draws basic shape objects.

You can middle mouse + drag to pan around and scroll to zoom.
Right click also toggles a demo animation that makes the tiles move around.

It has pretty good performance because it uses ggez's SpriteBatches for tile rendering. It could be better by caching and reusing the batches but I figured I'd leave that out to simplify a bit.

I made a nicer example map for testing locally but I'll make a separate PR for that I think.
Since the current example map is large and mostly empty, and this centers it in the window at the start, you need to zoom out or pan in order to see it (the other nicer map has a more reasonable size).

(this does not properly hook into ggez's filesystem for loading external tilesets/images as that would still require something like #100 I believe)

This could probably still use a bit more clean up and commenting

related: #134

@bjorn
Copy link
Member

bjorn commented Feb 14, 2022

It did not compile for me:

error: there is no argument named `fps`
  --> examples/ggez/main.rs:83:49
   |
83 |         let text = graphics::Text::new(format!("{fps:.0} fps"));
   |                                                 ^^^^^^^^

error: could not compile `tiled` due to previous error

Also, seems like we need to add libasound2-dev and libudev-dev to the apt install command.

@PieKing1215
Copy link
Member Author

error: there is no argument named fps

I just changed it so it should compile on older rust versions (that feature was only just added in rust 1.58)

Copy link
Contributor

@aleokdev aleokdev left a comment

Choose a reason for hiding this comment

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

I'd appreciate if you add an example description as doc comments to the main file (like in the SFML example). Apart from that I've got two comments:

examples/ggez/main.rs Outdated Show resolved Hide resolved
examples/ggez/map.rs Outdated Show resolved Hide resolved
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Great and smooth running example! It showed over 5000 FPS for me on my aging GTX 1060, and over 7000 FPS when caching the layer_batches. Nice animation too! I've just left some minor comments.

examples/ggez/main.rs Outdated Show resolved Hide resolved
examples/ggez/main.rs Outdated Show resolved Hide resolved
examples/ggez/main.rs Outdated Show resolved Hide resolved
examples/ggez/map.rs Outdated Show resolved Hide resolved
tiled::ObjectShape::Rect { width, height } => {
let bounds = graphics::Rect::new(object.x, object.y, *width, *height);
let shape =
graphics::Mesh::new_rectangle(ctx, graphics::DrawMode::stroke(2.0), bounds, graphics::Color::CYAN)?;
Copy link
Member

Choose a reason for hiding this comment

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

Note that there is also Object::get_tile, which would be nice to support as well. Of course then the object needs to be accessed through ObjectLayer::get_object instead of iterating ObjectLayerData::objects directly.

Since this might be a bit of a cludge, we could wait with supporting that until #158 is resolved.

examples/ggez/map.rs Outdated Show resolved Hide resolved
examples/ggez/map.rs Outdated Show resolved Hide resolved
examples/ggez/main.rs Outdated Show resolved Hide resolved
examples/ggez/map.rs Outdated Show resolved Hide resolved
@aleokdev aleokdev added the tests label Feb 23, 2022
@PieKing1215
Copy link
Member Author

Cleaned up some of the easy stuff, still some organization and structural things left to do, and I still need to merge master into here again

Comment on lines 75 to 80
for layer in &layer_batches {
// for each tileset in the layer
for batch in layer {
graphics::draw(ctx, batch, draw_param)?;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be flattened to a single vector if layer batches are sequentially placed by index

@bjorn
Copy link
Member

bjorn commented Mar 28, 2022

I've merged master and fixed the compile, and also adjusted main.rs to use the new Loader class. However, for me this example is currently just displaying a magic pink window. Any idea why I'm no longer seeing the map?

@PieKing1215 Do you think you could find some time to finalize this example? I think it'd be really nice to have this merged, and it would eventually also be able to demonstrate the VFS support that has been merged for 0.11 (#199).

@bjorn
Copy link
Member

bjorn commented Mar 28, 2022

@aleokdev It appears the clippy check needs also the dependencies installed. Maybe it would make sense to merge it with the build check?

@aleokdev
Copy link
Contributor

The CI deps can be added in this PR as they're needed by this example. Is that what you're referring to? I'm not sure if I understood you.

@bjorn
Copy link
Member

bjorn commented Mar 28, 2022

The CI deps can be added in this PR as they're needed by this example. Is that what you're referring to? I'm not sure if I understood you.

I just mean that it's somewhat annoying if we need to install the dependencies in two places in the workflow, just because the build and clippy checks happen on separate virtual machines. It might be better to just perform those checks at two separate steps on the same machine.

@aleokdev
Copy link
Contributor

aleokdev commented Mar 28, 2022

Oh, sure, that makes sense. However, I'd like for them to be treated as separate checks so that we can see at a glance what's wrong. Is that possible?

@bjorn
Copy link
Member

bjorn commented Mar 28, 2022

Oh, sure, that makes sense. However, I'd like for them to be treated as separate checks so that we can see at a glance what's wrong. Is that possible?

I agree that would be nice, but I have no idea whether this is possible. Maybe it can be done with this github-checks action.

@PieKing1215
Copy link
Member Author

@PieKing1215 Do you think you could find some time to finalize this example? I think it'd be really nice to have this merged, and it would eventually also be able to demonstrate the VFS support that has been merged for 0.11 (#199).

Yeah sorry about the lack of progress on this, I've been particularly busy recently but I should be able to dedicate some time to adding the rest of the suggested things.
(Thanks for merging master btw)

However, for me this example is currently just displaying a magic pink window. Any idea why I'm no longer seeing the map?

Did you try zooming out? It centers the map at the start but the test map's size is way too big and mostly empty so you can't see anything without zooming/panning (should probably fix this)

@bjorn
Copy link
Member

bjorn commented Mar 28, 2022

Did you try zooming out? It centers the map at the start but the test map's size is way too big and mostly empty so you can't see anything without zooming/panning (should probably fix this)

Heh, it had been a while since I had run this example, and I had just forgotten about the need to zoom out, whoops! But yes, that should definitely be fixed.

Cache gets invalidated when panning/zooming or every frame while animating
The performance improvement would be more noticeable if there were more layers & more tilesets per layer
@PieKing1215
Copy link
Member Author

The main things I can think of that are unsupported in this right now are:

  • Tile objects
  • Infinite maps
  • ImageLayer
  • GroupLayer

I plan on doing tile objects already, but I'm not sure about the other stuff. Idk how all-encompassing we want this example to be.
Also should I do VFS in this PR (and change base branch to 0.11) or leave that for the future?

@bjorn
Copy link
Member

bjorn commented Apr 3, 2022

I plan on doing tile objects already, but I'm not sure about the other stuff. Idk how all-encompassing we want this example to be.

I think tile objects would be good to cover, but the other stuff is not so important.

Also should I do VFS in this PR (and change base branch to 0.11) or leave that for the future?

I would suggest to target 0.10 first and update the example after merging it to 0.11, unless the lack of using VFS is really setting a bad example and we'd rather want to wait till 0.11 to show how to use this crate with ggez at all.

@aleokdev
Copy link
Contributor

aleokdev commented Apr 3, 2022

I'd target 0.11 to be honest, VFS is a huge deal with ggez (Plus 0.11's interface is basically finished at this point)

@aleokdev
Copy link
Contributor

Should we base this on 0.11 then?

@bjorn
Copy link
Member

bjorn commented Apr 12, 2022

@aleokdev It's fine with me too.

@aleokdev aleokdev changed the base branch from master to 0.11 April 12, 2022 14:26
@aleokdev
Copy link
Contributor

@PieKing1215 I'm basing this onto 0.11 then; You'll be able to use custom loaders now. My platformer template repo has an example of their usage:

// Need to do newtype to implement ResourceReader for ggez's filesystem
// FIXME: This would greatly improve with separated subcontexts (ggez 0.8.0)
pub struct FsContext<'ctx>(pub &'ctx mut ggez::Context);

impl tiled::ResourceReader for FsContext<'_> {
    type Resource = filesystem::File;

    type Error = GameError;

    fn read_from(
        &mut self,
        path: &std::path::Path,
    ) -> std::result::Result<Self::Resource, Self::Error> {
        filesystem::open(&self.0, path)
    }
}

Then you can simply

let mut loader = tiled::Loader::with_cache_and_reader(
     tiled::DefaultResourceCache::new(),
     FsContext(ctx),
);

And not worry about paths anymore. When tiled needs to load any supplementary data, it'll use ggez's filesystem to do so.

@PieKing1215
Copy link
Member Author

Cool, I'll take a look later then.

aleokdev and others added 5 commits May 10, 2022 10:24
thread 'main' panicked at '`"pkg-config" "--libs" "--cflags" "alsa"` did not exit successfully: exit status: 1
error: could not find system library 'alsa' required by the 'alsa-sys' crate
"Package libudev was not found in the pkg-config search path."
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

The map could be nicer, but I think the example is good to go now.

Btw, even if we have all done some edits in this PR and it has some history, I would still suggest we squash it all into a single commit, since the various changes are not really relevant on their own.

@aleokdev aleokdev merged commit ea91808 into mapeditor:next May 10, 2022
bjorn added a commit that referenced this pull request May 19, 2022
* Rendering example for ggez

* Update ggez example

* Refactoring for ggez example

* Re-add example entry I accidently removed

* Add note about caching layer_batches in ggez ex

* Update contributors list

* Change format string in ggez example to work on older rust versions

* Add libasound2-dev and libudev-dev to rust workflow apt install

* ggez: use CARGO_MANIFEST_DIR

Co-authored-by: Alejandro Perea <alexpro820@gmail.com>

* ggez: destructure some variables

Co-authored-by: Thorbjørn Lindeijer <bjorn@lindeijer.nl>

* ggez: more destructuring

* ggez: make left mouse also drag

* ggez: reduce time_since_start calls, make get_tile_rect return Rect

* Use the Loader

* Refactor some minor things

* Don't center the map by default

* Add batch caching to ggez example

Cache gets invalidated when panning/zooming or every frame while animating
The performance improvement would be more noticeable if there were more layers & more tilesets per layer

* Cargo fmt

* Polish

* Fixed 'bounds' function and removed unused import

* Apparently clippy check now needs alsa

thread 'main' panicked at '`"pkg-config" "--libs" "--cflags" "alsa"` did not exit successfully: exit status: 1
error: could not find system library 'alsa' required by the 'alsa-sys' crate

* Also install udev dependency

"Package libudev was not found in the pkg-config search path."

Co-authored-by: Alejandro Perea <alexpro820@gmail.com>
Co-authored-by: Thorbjørn Lindeijer <bjorn@lindeijer.nl>
Co-authored-by: aleokdev <aleok.inf@gmail.com>
@bjorn bjorn mentioned this pull request May 19, 2022
bjorn added a commit that referenced this pull request May 19, 2022
* Rendering example for ggez

* Update ggez example

* Refactoring for ggez example

* Re-add example entry I accidently removed

* Add note about caching layer_batches in ggez ex

* Update contributors list

* Change format string in ggez example to work on older rust versions

* Add libasound2-dev and libudev-dev to rust workflow apt install

* ggez: use CARGO_MANIFEST_DIR

Co-authored-by: Alejandro Perea <alexpro820@gmail.com>

* ggez: destructure some variables

Co-authored-by: Thorbjørn Lindeijer <bjorn@lindeijer.nl>

* ggez: more destructuring

* ggez: make left mouse also drag

* ggez: reduce time_since_start calls, make get_tile_rect return Rect

* Use the Loader

* Refactor some minor things

* Don't center the map by default

* Add batch caching to ggez example

Cache gets invalidated when panning/zooming or every frame while animating
The performance improvement would be more noticeable if there were more layers & more tilesets per layer

* Cargo fmt

* Polish

* Fixed 'bounds' function and removed unused import

* Apparently clippy check now needs alsa

thread 'main' panicked at '`"pkg-config" "--libs" "--cflags" "alsa"` did not exit successfully: exit status: 1
error: could not find system library 'alsa' required by the 'alsa-sys' crate

* Also install udev dependency

"Package libudev was not found in the pkg-config search path."

Co-authored-by: Alejandro Perea <alexpro820@gmail.com>
Co-authored-by: Thorbjørn Lindeijer <bjorn@lindeijer.nl>
Co-authored-by: aleokdev <aleok.inf@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants