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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions Sources/Core/API/TextureManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ public final class TextureManager {
/// Which mode to use in case an error was encountered when loading a texture (default = .ignore)
/// This is considered only in DEBUG mode.
public var errorMode: ErrorMode = .ignore
/// 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)

internal private(set) var cache = [String : LoadedTexture]()

Expand All @@ -47,8 +50,11 @@ public final class TextureManager {

// MARK: - Internal

internal func load(_ texture: Texture, namePrefix additionalNamePrefix: String?, scale: Int?) -> LoadedTexture? {
internal func load(_ texture: Texture, namePrefix additionalNamePrefix: String?,
scale: Int?, otherAllowedScales: [Int]? = nil) -> LoadedTexture? {
let scale = scale ?? defaultScale
var otherAllowedScales = otherAllowedScales ?? allowedScales(beside: scale)

let format = texture.format ?? defaultFormat
var name = texture.name

Expand Down Expand Up @@ -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

guard scale > 1 else {
guard otherAllowedScales.count > 0 else {
#if DEBUG
let errorMessage = "Texture image named '\(name)' could not be found"

Expand All @@ -93,13 +99,24 @@ public final class TextureManager {
return nil
}

return load(texture, namePrefix: namePrefix, scale: scale - 1)
return load(texture, namePrefix: namePrefix,
scale: otherAllowedScales.removeLast(), otherAllowedScales: otherAllowedScales)
}

let texture = LoadedTexture(image: image, scale: scale)
cache[cacheKey] = texture
return texture
}

// MARK: - Private

func allowedScales(beside defaultValue: Int?) -> [Int] {
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

.filter { defaultValue == nil || $0 != defaultValue }
}
}

public extension TextureManager {
Expand Down
4 changes: 4 additions & 0 deletions Sources/Core/Internal/Screen-iOS.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,8 @@ internal extension Screen {
static var mainScreenScale: Metric {
return main.scale
}

static var maxScreenScale: Metric {
return 3
}
}
4 changes: 4 additions & 0 deletions Sources/Core/Internal/Screen-macOS.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,8 @@ internal extension Screen {
static var mainScreenScale: CGFloat {
return Screen.main?.backingScaleFactor ?? 1
}

static var maxScreenScale: Metric {
return 2
}
}
25 changes: 25 additions & 0 deletions Tests/ImagineEngineTests/ActorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ final class ActorTests: XCTestCase {
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",
Expand All @@ -117,10 +119,17 @@ final class ActorTests: XCTestCase {
actor.animation = newAnimation

game.update()

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

func testDefaultAnimationTextureFormatIsPNG() {
let imageSize = Size(width: 200, height: 150)
let image = ImageMockFactory.makeImage(withSize: imageSize)

game.textureImageLoader.images["Animation/0.png"] = image.cgImage
game.textureImageLoader.images["Animation/1.png"] = image.cgImage

var animation = Animation(
name: "Animation",
frameCount: 2,
Expand All @@ -138,6 +147,12 @@ final class ActorTests: XCTestCase {
}

func testAnimatingWithJPGTextures() {
let imageSize = Size(width: 200, height: 150)
let image = ImageMockFactory.makeImage(withSize: imageSize)

game.textureImageLoader.images["Animation/0.jpg"] = image.cgImage
game.textureImageLoader.images["Animation/1.jpg"] = image.cgImage

var animation = Animation(
name: "Animation",
frameCount: 2,
Expand All @@ -156,12 +171,22 @@ final class ActorTests: XCTestCase {
}

func testInitializingWithTextureName() {
let imageSize = Size(width: 200, height: 150)
let image = ImageMockFactory.makeImage(withSize: imageSize)

game.textureImageLoader.images["SomeTexture.png"] = image.cgImage

let actor = Actor(textureNamed: "SomeTexture", scale: 1)
game.scene.add(actor)
XCTAssertEqual(game.textureImageLoader.imageNames, ["SomeTexture.png"])
}

func testTextureNamePrefix() {
let imageSize = Size(width: 200, height: 150)
let image = ImageMockFactory.makeImage(withSize: imageSize)

game.textureImageLoader.images["PrefixTexture.png"] = image.cgImage

actor.textureNamePrefix = "Prefix"
actor.animation = Animation(textureNamed: "Texture", scale: 1)

Expand Down
4 changes: 4 additions & 0 deletions Tests/ImagineEngineTests/TextureManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,17 @@ class TextureManagerTests: XCTestCase {
}

func testApplyingTextureNamePrefix() {
let image = makeImage()
imageLoader.images["PrefixTexture.png"] = image

let texture = Texture(name: "Texture")
manager.namePrefix = "Prefix"

_ = manager.load(texture, namePrefix: nil, scale: 1)
XCTAssertEqual(imageLoader.imageNames, ["PrefixTexture.png"])

// When an additional name prefix is passed in, both prefixes should be applied in sequence
imageLoader.images["PrefixSecondTexture.png"] = image
_ = manager.load(texture, namePrefix: "Second", scale: 1)
XCTAssertEqual(imageLoader.imageNames, ["PrefixTexture.png", "PrefixSecondTexture.png"])
}
Expand Down