consulo / consulo-unity3d

Frameworks: Unity3D
Apache License 2.0
103 stars 10 forks source link

Consulo side for accepting scripting defines from Unity plugin #85

Closed electrowolff closed 8 years ago

electrowolff commented 8 years ago

Next step will be to remove the existing system for variables (version & build target) and add a fetch on project import

VISTALL commented 8 years ago

Hi. First of all, thanks for trying improve unity integration. But

Unity3dRootModuleExtension is only object for read, for write need change object of type Unity3dRootMutableModuleExtension (for it need create modification model of Module)

Project importer always create Unity targets as new configuration layer, and user can switch for it. For example

#if UNITY_WINDOWS
using Windows; // this will be disabled at Editor layer
#elif UNITY_EDITOR
using Editor
#endif

some code

For default, you staying at Editor layer. But you can switch to other layer, and C# preprocessor variables will be changed.

As i known, user defined variables are stored in different file at file system. Maybe read it while project importing ?

VISTALL commented 8 years ago

As i see, settings stored in ProjectSettings/ProjectSettings.asset

We can read it while improing

electrowolff commented 8 years ago

Reading from the project settings will only capture the user defined variables, my import system captures ALL defined preprocessor variables including UNITY_WINDOWS, UNITY_EDITOR, all the version-based defines, AND the user defined variables. There's something like 73 built-in variables in Unity 5.3

If the user adds any defines or changes build targets in Unity, it'll recompile and automatically sync the variables in Consulo to be consistent with Unity.

With this change you would get rid of the need for Unity3dDefineByVersion and Unity3dTarget

Does layer switching do anything other than set the variables?

VISTALL commented 8 years ago

Nope. But as i show, it will be hard edit code with for different platforms. UNITY_EDITOR will be always enabled, and UNITY_ANDROID - always disabled.

Often many code hidden via UNITY_ANDROID(or UNITY_IOS)

VISTALL commented 8 years ago

Currenly layers are very draft feature - in next version i will improve it. But not need ignore it at all

electrowolff commented 8 years ago

UNITY_ANDROID is enabled if you switch the build target in Unity to be an Android project. That's the way the defines work in Unity, and every Unity script editor follows that.

Having the editor handle scripting defines independently of how Unity itself is declaring them is risky and very unsustainable.

VISTALL commented 8 years ago

Yeah - but user can import project, without Unity Editor, and will have problems with defines.

Providing new action - is very risky, due even now users not often use Re-import project

electrowolff commented 8 years ago

With my changes the only thing you would need to do to keep the defines in sync with Unity would just be to have Unity open -- it syncs automatically if the defines change. Re-importing the project is not required. It's not an unreasonable dependency for a Unity script editor to require Unity to be running ;)

There's not a perfectly correct solution for importing a Unity project without Unity running, as you can't know what the correct defines are at that point. If there's an existing .csproj file, there's a DefineConstants entry that you could read from which might be close. However, I think that file will only exist if you've previously run a different editor like VSCode or Visual Studio.

VISTALL commented 8 years ago

For example:

I import Unity project (without Editor). Without default defines - that sucks.

You need open Editor and call - new action.

You known about this (me too), but other users not. It will some instruction for it.

Anyway if i accept it. Need do few changes:

VISTALL commented 8 years ago

With first two i can help. But i really worried about how intro this change to users

electrowolff commented 8 years ago

I think importing a project without having Unity running is a real edge case. All Unity devs I know open the script editor by either double clicking a script/console line in Unity or by using Assets > Open C# Project. I've never heard of someone editing scripts without having Unity running, I wouldn't worry too much about that situation. I only ever expect the script editor to reflect the current state of the Unity editor.

You could handle the no defines case by checking for the plugin connection. If it's not available then give the user a prompt or tooltip that they should install the plugin and run Unity before editing scripts? You could also keep the existing defines code as a fallback for when the Unity syncing isn't available?

VISTALL commented 8 years ago

But I heard (too often, sometimes when Unity is not worked on OS, linux before beta).

I think we can remove new action, and replace it with additional logic inside Re-import project - like ping-pong. And how error if no editor opened or wrong project.

electrowolff commented 8 years ago

Ah interesting case, I hadn't thought of that. There could certainly be a "load defaults" option on import if Unity isn't available. Like I said though, it can be tricky to have good coverage on what "defaults" are as at any given point Unity has 70+ defines set.

I was hoping to add the defines fetch to the project import as well yes. I like it also being sent on script compile like it is right now, as in my project we change the scripting defines fairly regularly and it's nice to have them synced automatically back to Consulo. I wouldn't want to have to manually re-import the project any time the defines are changed in Unity.

What if we add a new layer, and call it Automatic and set that to the default on project import if the Unity plugin is available? Then you could still switch to Editor/Windows/etc with manual lists of defines, and still have the nice automatic layer with synced defines.

VISTALL commented 8 years ago

I think we can handle ProjectSettings.asset change, and call Sync. Sometimes it will useless action call, but only sometimes. Platform switch settings stored in ProjectSettings too ? (i can check it after few hours)

Custom layers is not supported (not popular feature due few major bugs, that why i said that we can drop it)

electrowolff commented 8 years ago

Hmm yes ProjectSettings should change any time the user changes scripting defines. I don't think the build target is actually saved in the Assets folder. I think it's actually determined by something in Library but I'm not sure.

Any strong reason to not trigger the sync off of Unity finishing compiling? That should trigger any time the user's defines change, or the built target is switched.

You could also poll EditorUserBuildSettings.activeScriptCompilationDefines or use EditorUserBuildSettings.activeBuildTargetChanged to listen for build target changes.

VISTALL commented 8 years ago

We can call Sync out of Consulo. But not often. It analyze all project, and recreating it.

Ou.

EditorUserBuildSettings.activeBuildTargetChanged we can handle it, and notify Consulo with different defines.

VISTALL commented 8 years ago

And what we do? I sure - adding fetch defines at project import is good idea, and need add handle for BuildTarget change too.

UnityTarget - this class is not needed at all (define only UNITY_EDITOR)

Unity3dDefineByVersion - leave it as failback while importing

Sure - i can implement it, at this week.

electrowolff commented 8 years ago

Yeah I don't have a ton of time to contribute, so if you want to take what I've started and wrap it into your own changes this week that works for me.

I'm pretty excited about this project. I develop on OSX and the existing editors are so bad!

VISTALL commented 8 years ago

Hi. I implemented fetching defines from UnityEditor while importing.

Changing Build Target change defines in Consulo too.

If need re-fetch defines - need call Re-Import