Skip to content

GLTFUnarchiver + GLTFSceneSource: add embedExternalImages option #34

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

GLTFUnarchiver + GLTFSceneSource: add embedExternalImages option #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions Source/Common/GLTFSceneSource.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,22 @@ public class GLTFSceneSource : SCNSceneSource {
super.init()
}

public convenience init(path: String, options: [SCNSceneSource.LoadingOption : Any]? = nil, extensions: [String:Codable.Type]? = nil) throws {
public convenience init(path: String, options: [SCNSceneSource.LoadingOption : Any]? = nil, extensions: [String:Codable.Type]? = nil, embedExternalImages: Bool = true) throws {
self.init()

let loader = try GLTFUnarchiver(path: path, extensions: extensions)
let loader = try GLTFUnarchiver(path: path, extensions: extensions, embedExternalImages: embedExternalImages)
self.loader = loader
}

public override convenience init(url: URL, options: [SCNSceneSource.LoadingOption : Any]? = nil) {
self.init(url: url, options: options, extensions: nil)
self.init(url: url, options: options, extensions: nil, embedExternalImages: true)
}

public convenience init(url: URL, options: [SCNSceneSource.LoadingOption : Any]?, extensions: [String:Codable.Type]?) {
public convenience init(url: URL, options: [SCNSceneSource.LoadingOption : Any]?, extensions: [String:Codable.Type]?, embedExternalImages: Bool = true) {
self.init()

do {
self.loader = try GLTFUnarchiver(url: url, extensions: extensions)
self.loader = try GLTFUnarchiver(url: url, extensions: extensions, embedExternalImages: embedExternalImages)
} catch {
print("\(error.localizedDescription)")
}
Expand Down
28 changes: 19 additions & 9 deletions Source/Common/GLTFUnarchiver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,16 @@ public class GLTFUnarchiver {
private var buffers: [Data?] = []
private var materials: [SCNMaterial?] = []
private var textures: [SCNMaterialProperty?] = []
private var images: [Image?] = []
// An array of Image or URL types
private var images: [Any?] = []
Copy link
Author

Choose a reason for hiding this comment

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

Please note that I’m new to Swift, and my most recent experience with typed languages is TypeScript. Unfortunately I couldn’t declare this as an array of Image | URL types. If Any is unacceptable, I would consider this instead:

Suggested change
private var images: [Any?] = []
private var images: [Image?] = []
private var imageUrls: [URL?] = []

private var maxAnimationDuration: CFTimeInterval = 0.0
private var embedExternalImages: Bool = true

#if !os(watchOS)
private var workingAnimationGroup: CAAnimationGroup! = nil
#endif

convenience public init(path: String, extensions: [String:Codable.Type]? = nil) throws {
convenience public init(path: String, extensions: [String:Codable.Type]? = nil, embedExternalImages: Bool = true) throws {
var url: URL?
if let mainPath = Bundle.main.path(forResource: path, ofType: "") {
url = URL(fileURLWithPath: mainPath)
Expand All @@ -52,16 +54,18 @@ public class GLTFUnarchiver {
guard let _url = url else {
throw URLError(.fileDoesNotExist)
}
try self.init(url: _url, extensions: extensions)
try self.init(url: _url, extensions: extensions, embedExternalImages: embedExternalImages)
}

convenience public init(url: URL, extensions: [String:Codable.Type]? = nil) throws {
convenience public init(url: URL, extensions: [String:Codable.Type]? = nil, embedExternalImages: Bool = true) throws {
let data = try Data(contentsOf: url)
try self.init(data: data, extensions: extensions)
try self.init(data: data, extensions: extensions, embedExternalImages: embedExternalImages)
self.directoryPath = url.deletingLastPathComponent()
}

public init(data: Data, extensions: [String:Codable.Type]? = nil) throws {
public init(data: Data, extensions: [String:Codable.Type]? = nil, embedExternalImages: Bool = true) throws {
self.embedExternalImages = embedExternalImages

let decoder = JSONDecoder()
var _extensions = extensionList
extensions?.forEach { (ext) in _extensions[ext.key] = ext.value }
Expand Down Expand Up @@ -771,7 +775,8 @@ public class GLTFUnarchiver {
return valueArray
}

private func loadImage(index: Int) throws -> Image {
// Returns either an Image or a URL
private func loadImage(index: Int) throws -> Any {
Copy link
Author

@kohlmannj kohlmannj Apr 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Similar to the above comment regarding Any, I am willing to revise this part of the code to avoid Any as the return type. I would consider factoring out the shared logic here in loadImage() and then create a second private func loadImageUrl(index: Int) throws -> URL method.

guard index < self.images.count else {
throw GLTFUnarchiveError.DataInconsistent("loadImage: out of index: \(index) < \(self.images.count)")
}
Expand All @@ -785,7 +790,8 @@ public class GLTFUnarchiver {
}
let glImage = images[index]

var image: Image?
// An Image or a URL
var image: Any?
Copy link
Author

Choose a reason for hiding this comment

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

Same comment regarding Any usage as the above comments (first comment, second comment).

if let uri = glImage.uri {
if let base64Str = self.getBase64Str(from: uri) {
guard let data = Data(base64Encoded: base64Str) else {
Expand All @@ -794,7 +800,11 @@ public class GLTFUnarchiver {
image = try loadImageData(from: data)
} else {
let url = URL(fileURLWithPath: uri, relativeTo: self.directoryPath)
image = try loadImageFile(from: url)
// Even if we shouldn't embed external images, try loading the image file for now
// TODO: figure out how to check if `url` exists on-disk rather than using `loadImageFile()` to do this
let loadedImage = try loadImageFile(from: url)
Copy link
Author

Choose a reason for hiding this comment

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

This is the most awkward part of this revision. As indicated in the comments, I’m essentially using loadImageFile() as a test to see if the file exists on-disk. There are a few ways to improve this:

  • Add a helper function to simply check if the image URL exists on-disk when self.embedExternalImages is false
  • Call loadImageFile() only when self.embedExternalImages is true

// Pass thru the image `url` instead of `loadedImage` when `self.embedExternalImages` is `false`
image = self.embedExternalImages ? loadedImage : uri
}
} else if let bufferViewIndex = glImage.bufferView {
let bufferView = try self.loadBufferView(index: bufferViewIndex)
Expand Down