Conversation
|
Oh wow thanks for looking into this! I'll take a look this week if I find time. At a glance it looks lovely (so much less code!!) |
|
Totally! I see your message in #ink-unity-integration-dev. I'll keep watch for questions/comments there too. Thanks! |
…ince ink files aren't constucted at runtime anymore
|
@tomkail Okay I'm moving this out of draft as I've cleaned up the known things I needed to do, but I suspect there may be things I've missed since this was such a huge change. Feel free to tag me on any changes you want after doing a review. I'll loop back when I have the time 🤘 |
|
Amazing! Annoyingly nobody has checked this out. How about we stick it on a
beta branch and publish it there? Maybe that’ll encourage people.
…On Mon, Oct 7, 2024 at 00:49 Alex Larioza ***@***.***> wrote:
@tomkail <https://github.com/tomkail> Okay I'm moving this out of draft
as I've cleaned up the known things I needed to do, but I suspect there may
be things I've missed since this was such a huge change. Feel free to tag
me on any changes you want after doing a review. I'll loop back when I have
the time 🤘
—
Reply to this email directly, view it on GitHub
<#205 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJR3UED36NFMMKTA7MLXJDZ2HD67AVCNFSM6AAAAABNPMJ5EWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJVGY2TKOJTGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Sounds good to me! I'd rather give more people more time to test it out before its merged into main 😅 I don't appear to have the option to change the base branch to a new (non-existing) branch tho. If you make a beta branch I can update the PR! |
|
I forgot to update the docs, so I snuck another commit in 👍 |
| DefaultAsset asset = AssetDatabase.LoadAssetAtPath<DefaultAsset>(path); | ||
| DrawInkFile(InkLibrary.GetInkFileWithFile(asset), rect); | ||
| InkFile asset = AssetDatabase.LoadAssetAtPath<InkFile>(path); | ||
| DrawInkFile(asset, rect); |
There was a problem hiding this comment.
InkFile is now a Scriptable Object.
No OnDrawProjectWindowItem would be needed if we assign a custom icon to InkFile.cs from the inspector
|
Seems I didn't explain the idea that well...
importer wouldn't have to successfully import ink file if its missing variable declarations. Let's say we have a project with following files:
Let's say we import the project and assets are imported in unfavorable order for us where File B is being being imported before file A:
Importer design issueEven if the above solution fixes the issue I would actually change the way importer works. We're trying to import File B but its missing variable X? No problem, it should report importer warning but still succeed at file import. The result will have "{variable name}" in places where the variable should be. As a source of inspiration I recommend looking at Unity UXML and USS files, how they're imported and how they deal with references between each other. |
|
Ahh thanks for the explanation! I'm still not sure what the additional InkVariable objects get us, but I think the second half of your last post hits on what I've been trying to describe:
If I'm understanding correctly, I think we're on the same page here. Specifically, if an Ink file fails to compile (and by compile I mean compile via the Ink compiler) it should still be successfully imported into Unity. However, what I'm still unsure about is the developer experience for this. Going with your example, if File B is imported first and logs a warning regarding the compilation failure to the Unity console, how should we follow up once File A is imported? Additionally, importing File A still doesn't fix the compilation of File B because File B is not intended to be compiled as a main file. I'm starting to feel more strongly for proposal #2 from my earlier comment as I think this is likely the same issue the original developers faced and why the master file toggle was added. @tomkail Do you have any historical context you could share? |
…s that aren't intended to be compiled standalone
|
Hey all! What is the status of this PR? Reading through the thread, it seems like this was slowed down by the |
|
I’ve not been working on any big ink projects lately and I’m worried about
merging this without a lot of testing. Please do give it a spin and see how
it goes for you!
…On Wed, Aug 27, 2025 at 17:44 ARPP3 ***@***.***> wrote:
*ARPP3* left a comment (inkle/ink-unity-integration#205)
<#205 (comment)>
Hey all! What is the status of this PR?
Reading through the thread, it seems like this was slowed down by the
INCLUDE statements. Testing it out locally, it seems super promising. I
also was running into frustrations with the sidekick files and seeing the
single asset feels really like a great step for this package. I actually
only came across this fork after trying to do the same myself 😅 Would be a
shame for this to become outdated,
@SHiLLySiT <https://github.com/SHiLLySiT> @tomkail
<https://github.com/tomkail> @ErnSur <https://github.com/ErnSur>
—
Reply to this email directly, view it on GitHub
<#205 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJR3UGREKJGO64PCAZNUGT3PXN5TAVCNFSM6AAAAABNPMJ5EWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTEMRYHE3DANJRHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
We've been using my fork at work for two projects, with one of them being quite sizeable. I had been waiting to confirm until it shipped in a few months, but at this point the team has been using it for over a year so I'd say it's pretty stable. I think the main impact is people with existing projects that want to upgrade. It's going to probably require a lot of manual changes for them to migrate to this new version. |
|
Amazing!
Can we ship this but make it disabled by default? That’d be the best place
to start; we can then have a button that enables it and updates the users
project.
We have an editor window that I can force to pop up when ink updates, so we
can use that to inform people.
Can you list the changes somebody upgrading would need to know about?
…On Wed, Aug 27, 2025 at 20:11 Alex Larioza ***@***.***> wrote:
*SHiLLySiT* left a comment (inkle/ink-unity-integration#205)
<#205 (comment)>
We've been using my fork at work for two projects, with one of them being
quite sizeable. I had been waiting to confirm until it shipped in a few
months, but at this point the team has been using it for over a year so I'd
say it's pretty stable.
I think the main impact is people with existing projects that want to
upgrade. It's going to probably require a lot of manual changes to work
with this new version.
—
Reply to this email directly, view it on GitHub
<#205 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJR3UGQDLGYQ5L5NOHSDPL3PX7G7AVCNFSM6AAAAABNPMJ5EWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTEMRZGQ2DCNBZHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@tomkail I wouldn't recommend putting this behind a feature toggle. We'd need to restore all the now deleted code and refactor the refactoring I did to support both systems 😅 I'm also not quite sure what Unity would do with all the imported Ink scripts when the custom importer is disabled. I'd recommend a major version bump and include a migration guide. I can write the guide - would you prefer it as a Markdown file in the repo, a comment in this PR, or somewhere else? |
# Conflicts: # Packages/Ink/Editor/Core/Ink Library/InkLibrary.cs # Packages/Ink/Editor/Tools/Player Window/InkPlayerWindow.cs
|
Just catching up on this now. It looks really promising! I have some thoughts on the outstanding question about the best workflow for Unless I'm misunderstanding something, I don't think that exposing global variables as sub-assets would solve all of the problems of missing To my mind, thoroughly capturing, parsing, and resolving all these dependencies is outside the scope of the original intended change - and very welcome improvement! - here. I'd suggest that the pragmatic option is for the user to continue to explicitly identify which files are master files. As well as being a familiar concept and workflow for existing users, this would greatly reduce the scope of work required to land this update, and therefore its potential for bugs. If, in the future, there's a desire to build on this further and add some kind of automatic dependency resolution, that can be treated as a separate PR. |
|
@reallyfancy Hey thanks for jumping in here!
Yup! This is also where I landed, so we're in agreement there.
Ahh that is a great argument for keeping the old terminology. Its been awhile now so I don't quite remember my reasoning for the name change 🤔 I just updated the PR to rename the new toggle! |
|
@tomkail I also added some migration notes to the changelog to communicate changes to users. |
|
Thanks for getting back to me! With regard to the naming, there's generally a move away from calling things "master" in favour of maintaining/root/whatever makes contextual sense, and I'm definitely not opposed to that - but the exact name that feels like a decision for maintainers! The important consideration from my perspective is limiting change for users - in this case, letting them keep thinking in terms of opting in to standalone compilation, rather than opting out. Anyway, this is looking good! I'm happy to take a look at this, likely in a week or so, and experimentally port a medium-sized Ink project to it to see what happens. @tomkail I see that you expressed concerns about this change not being behind a feature flag, but @SHiLLySiT doesn't think that's the right fit for this code. It would be good to figure out what would give you the confidence to merge this. It's a big change but there are a lot of ways to derisk things like this - additional safety, additional testing, major version bump, migration tooling, and so on. I'm happy to pitch in to help this land, if we can come up with a plan! |
|
Thanks both! If you port your project and are happy with the results then I think we should merge it in with a major version bump. |
|
@tomkail Sorry for the slow reply, I caught that nasty winter virus that's doing the rounds and was out of action for a couple of weeks. That's great, thanks for confirming what works for you. I'll take a look at upgrading our project - I've got some downtime after Christmas - and I'll share my findings here. |
reallyfancy
left a comment
reallyfancy
left a comment
There was a problem hiding this comment.
Apologies that this took a while - I had an unexpectedly busy few weeks!
I'm afraid I've identified a couple of serious issues, but I've provided suggested fixes for both. I've also raised a couple of questions, and marked some minor style issues.
For context, I reviewed this both with a mini project that I spun up just for testing, and in the context of my current work project (which has a very large Ink source, with complex dependencies and custom tooling).
I'd still like to look closer at Editor performance and how this works with our automated build scripts, so this is a provisional review for now - but hopefully what I've raised so far is useful, and a good jumping off point for discussion!
reallyfancy
left a comment
reallyfancy
left a comment
There was a problem hiding this comment.
I had the chance to test this on a very large project, and it's such a big improvement! It solves so many issues caused by the old compilation system. Really great work!
I'm requesting one change that I consider a blocker for large projects, but it's a very small change. I'll be happy to approve after the blocker is addressed!
| [Tooltip("Set this to false to stop the Ink file from being compiled on import. This is intended for " + | ||
| "Ink files that aren't intended to be compiled as standalone files (aka master files) because they are included in other " + | ||
| "Ink files and require other files to be compiled correctly e.g. global variables are defined in another file.")] | ||
| private bool isMaster = true; |
There was a problem hiding this comment.
Blocker: this should default to false. In a typical project, only a single Ink file will be a master file, but there might be many more includes. When I tested it on a very large project with 100+ includes, it caused a very lengthy import process with 999+ error messages caused by trying and failing to compile .ink files standalone.
(With apologies, because I think it was me who suggested that this should default to true - that was based on a misunderstanding, and now that I've used this more I totally take it back!)
There was a problem hiding this comment.
@reallyfancy Oh whoops, actually you noted this should be false previously. I just forgot to flip it when I did the variable rename 😅
However I want to note that all of my projects actually do the opposite: there are multiple standalone stories that have a couple of shared includes i.e. we have multiple master files. Maybe I'm the weird one out here and what you have is actually typical, but I think it would be wiser to make this decision with a better idea of what a typical project is. Though I'm not sure the best way to go about gathering more data here 🤔
Something else to consider is that while the old workflow also allows users to indicate which files are master files (i.e. adding a file to includeFilesToCompileAsMasterFiles in the ink settings) it actually serves as an override to force include files to be treated as master files. Master files were automatically identified based on if they weren't used as an include in another file.
Unfortunately this requires two passes on importing: one to identify includes in files, and then one to correctly identify master and includes files based on the first pass. However, I'm not sure if the Unity importer would support this, so I'll have to do some digging. But if we can accomplish this, maybe this is the better route?
There was a problem hiding this comment.
Ahh looks like we could utilize GatherDependenciesFromSourceFile but this would require upgrading the project to at least 2022.3.
The project is currently on 2020.3.25 which also happens to be out of LTS. @tomkail could we also do a Unity upgrade in this PR? If so, any preferences on how far?
Alternatively I could open a new PR for the upgrade so we can get that merged in before this one.
There was a problem hiding this comment.
Oh, that method looks like it would be great if we could upgrade that far. Nice find!
And I agree that the unwanted compilation of files that aren't currently included anywhere is a pain, and it's nice to fix that. That's bitten me a couple of times too.
With regard to which path to choose here, I agree that streamlining for the common use case is a really good goal - but we should also consider what negative consequences the users would experience in both cases.
In my case, the Editor was unresponsive for about 10 minutes during the initial import (I've genuinely never seen it hang so long!), and it took me about 30 minutes to manually fix the 100+ affected files because the Editor was so sluggish due to the thousands of errors. It was a really unpleasant upgrade experience.
The negative consequences of someone with multiple master files would be that they have to manually locate those files and enable the checkbox, without the Editor being full of errors.
It might be that there's a third way here - perhaps a wizard on first run where you can pre-select, or perhaps retaining but deprecating some of the old code to automatically apply the previous setting? - I suspect those are out of scope, but it's worth kicking this one around I think!
| - JSON sidecar files are no longer generated. This means instead of referencing a JSON file in your scripts as a `TextAsset`, you can now reference the Ink file directly as a `InkFile`. This also means you don't need to remember commit both `.ink` and `.json` files, which can happen if you're testing/writing outside Unity with Inky. | ||
| - Due to the nature of scripted importers, Ink files no longer automatically handle identifying master and include files. This has the following impacts: | ||
| - Ink files no longer display links to related include or master files. | ||
| - The new InkImporter exposes a `isMaster` toggle which defaults to `true`. When enabled, Ink files are compiled when first imported and when modified. If you have an Ink file that is not meant to be a standalone file (i.e. a file with content that is not playable by itself, usually when its intended to be included in another Ink file) you should set this toggle to `false`. Otherwise such files will log Ink compilation errors to the Unity console. |
There was a problem hiding this comment.
If you update this to default to false, just a reminder to update the docs!
…he contained scripts also updated editor assembly to reference assemblies by GUID so future rename don't break references
5 participants
This aims to implement #53, which IMO, has some huge benefits:
InkFilereferences instead of genericTextAssetreferences in game code.This PR is currently in a proof of concept stage: it hasn't been fully tested, but loading and running ink files at runtime and in the Editor is working.
The original implementation was written before Unity supported custom scripted importers, so many of the systems are now obsolete. Since this is a pretty big change, I wanted to open a PR to get an initial review to get some initial thoughts from the maintainers.
Interested in all feedback, but the question at the top of my mind: Do we need to expose includes and master files in the editor? While Ink at a language level still has these concepts, I'm not sure how important it is to expose these in Unity if its no longer important for compilation?