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 initial externally stored levels support #9

Closed

Conversation

quasilyte
Copy link
Contributor

This feature will only work with Open(), since that function can easily figure out the file location
(it gets it as an argument).

I don't know how to make Read() work without there breaking its API.

Fixes #8

This feature will only work with `Open()`, since that
function can easily figure out the file location
(it gets it as an argument).

I don't know how to make `Read()` work without there
breaking its API.

Fixes SolarLune#8
"path/filepath"
"strconv"

"github.com/tidwall/gjson"
)

//LayerType constants indicating a Layer's type.
// LayerType constants indicating a Layer's type.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some changes like this are due to the gofmt on-save.
I hope it doesn't make this PR much harder to review.

It would be better to send them separately (if at all), maybe?

@SolarLune
Copy link
Owner

SolarLune commented Feb 21, 2024

Sorry for the late reply - gofmt changing formatting is perfectly fine, and can be added into this PR without issue.

I'm wondering, though, if it might be better to add the ability to read from a file system - that way, we could reliably know what the file system of an embedded or on-disk asset directory looks like to determine where external levels might be.

@kettek
Copy link

kettek commented Feb 21, 2024

Feel free to ignore this input as I haven't used this library, however it is relevant to SolarLune's previous statement.

Using one or more of the fs interfaces (https://pkg.go.dev/io/fs) would be the best and most idiomatic go choice. I personally don't like it when libraries always assume that read operations are done with the local filesystem. Traditionally, many games ship their assets in a compressed or obfuscated format, of which access to would be best implemented via an interface. If a local file system is always presumed, then some pretty bad kludges have to be implemented to give access.

@quasilyte
Copy link
Contributor Author

That is true. I usually have some sort of a file reader abstraction myself, which can read either from a local file system or an embedded storage (based on a filename prefix). I didn't want to break the API though. I've just started to use this library and I don't feel the courage to propose such changes. :D

@SolarLune
Copy link
Owner

Hello! I updated ldtkgo recently to add a function that can load a file from a file-system. Would you be interested in adapting your PR to use that new function now?

@quasilyte
Copy link
Contributor Author

Sure, I'll take a look.

@quasilyte
Copy link
Contributor Author

I'm not feeling confident about keeping this PR open anymore.
I still need this feature, but I'm not working on a project with ldtk levels anymore (although I might come back to it in a few months).
I'll close the PR and maybe someone can implement it (if not, I may send another PR when I have some time for it again).

@quasilyte quasilyte closed this Jun 15, 2024
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.

Empty Level.Layers when opening a 1.5.3 LDtk project
3 participants