Issue #149 textures with higher resolutions

[ghost]

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 🙄

[Johns-CI-Bot]

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

[Johns-CI-Bot]

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

Generated by 🚫 Danger

[JohnSundell]

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 😄

[JohnSundell]

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?

[ghost]

@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 :/

[ghost]

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

[ghost]

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"]")

[ghost]

So I have probably broken something

[JohnSundell]

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