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

Issue #149 textures with higher resolutions #163

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

Issue #149 textures with higher resolutions #163

wants to merge 6 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 29, 2018

Issue #149

OK, it may be that it's more a code review request rather than an actually pull request, but still. Having a discussion is also great.

Since we want to look for higher resolutions first, we need to set a maximum limit for the scale.

    /// The maximum screen scale value allowed
    /// It's used when there are no assets for textures in proper scales are found
    private let maxAllowedScale: Int = Int(Screen.maxScreenScale)

I've added that to Screen.maxScreenScale, but there may be a better way to do this.

Also, there most probably is something I haven't understood in the tests and I may have broken them. Or fixed them. There is an issue with loading a texture without having created an image - it starts looking for other resolutions and thus adding more filenames to imageLoader.imageNames and I'm pretty sure there should be a better way.

I may have missed something obvious 🙄

@@ -76,7 +82,7 @@ public final class TextureManager {
}

guard let image = imageLoader.loadImageForTexture(named: name, scale: scale, format: format) else {

Choose a reason for hiding this comment

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

⚠️ Add a newline after guard statement to make it easier to read

@Johns-CI-Bot
Copy link

1 Warning
⚠️ Tests/ImagineEngineTests/ActorTests.swift#L727 - TODOs should be avoided (Needed until we find a better …).

Generated by 🚫 Danger

Copy link
Owner

@JohnSundell JohnSundell left a comment

Choose a reason for hiding this comment

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

Thanks for this @Loyolny! 🙌 Put a comment regarding performance inline. I'm also curious why the existing tests needed to be modified as part of implementing this? 🤔 We should only need to add more tests, not modify the existing ones when adding functionality like this? Let me know what you think 😄

let minScale = 1
let maxScale = maxAllowedScale

return [Int](minScale...maxScale)
Copy link
Owner

Choose a reason for hiding this comment

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

Since this code is in a very performance-ciritical path (loading textures, which can occur many times per frame), I don't think we should create multiple collections like this. Instead, how about simply iterating by incrementing the scale up to the maximum point, just like we do when scaling downwards?

Copy link
Author

@ghost ghost Feb 5, 2018

Choose a reason for hiding this comment

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

@JohnSundell, I'm sorry, I somehow missed your report 😏

So you're saying I should just iterate up, and then down to 1?
I agree that performance may and probably will be the issue here. I'll try to fix that.

As for the tests, it's like I've stated in the opening of the pull request. They got broken because they've been searching for images that did not exist, so multiple resolutions filenames were added to names collection, without any images corresponding to them.

As you say, I also think that new code should not break the tests, but this one broke. Also, I've forgotten to actually add test for the new scaling :/

Copy link
Author

@ghost ghost Feb 5, 2018

Choose a reason for hiding this comment

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

I just have this suspicion that the tests worked because the scaling went only downwards.

Copy link
Author

Choose a reason for hiding this comment

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

What I mean is, when the test looked like this:

func testAnimatingWithSpriteSheet() {
        let imageSize = Size(width: 300, height: 100)
        let image = ImageMockFactory.makeImage(withSize: imageSize)
        game.textureImageLoader.images["sheet.png"] = image.cgImage

        var animation = Animation(
            spriteSheetNamed: "sheet",
            frameCount: 6,
            rowCount: 2,
            frameDuration: 1
        )
        animation.textureScale = 1

        let actor = Actor()
        actor.animation = animation
        game.scene.add(actor)
        XCTAssertEqual(actor.size, Size(width: 100, height: 50))

        game.timeTraveler.travel(by: 1)
        game.update()
        XCTAssertEqual(actor.size, Size(width: 100, height: 50))

//        game.textureImageLoader.images["sheet2.png"] = image.cgImage

        // Assigning new sprite sheet mid-animation should update the animation
        var newAnimation = Animation(
            spriteSheetNamed: "sheet2",
            frameCount: 6,
            rowCount: 2,
            frameDuration: 1
        )
        newAnimation.textureScale = 1
        actor.animation = newAnimation

        game.update()

        XCTAssertEqual(game.textureImageLoader.imageNames, ["sheet.png", "sheet2.png"])
    }

I got XCTAssertEqual failed: ("["sheet.png", "sheet2@3x.png", "sheet2.png", "sheet2@2x.png"]") is not equal to ("["sheet.png", "sheet2.png"]")

Copy link
Author

Choose a reason for hiding this comment

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

So I have probably broken something

@JohnSundell
Copy link
Owner

Let me know if you want any help on this @Loyolny 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants