AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.4k stars 291 forks source link

restructured folder structure #823

Closed Barsonax closed 4 years ago

Barsonax commented 4 years ago

Paths can now be set from inside the editor: image

they will be persisted in a EditorAppData.dat file.

@ilexp I also saw there is also a EditorUserData.xml file but decided not to put it in here since its userdata. Besides this file seems to use custom logic with a stringbuilder. Not sure why it doesnt use the standard logic. Is this just legacy?

Barsonax commented 4 years ago

buildoutput still assumes the old folder structure so its a bit less tidy when you open up duality as duality now creates folders in the parent folder.

I do think we have to move these paths to a settings file as that makes it much easier to clean this up.

ilexp commented 4 years ago

Besides this file seems to use custom logic with a stringbuilder. Not sure why it doesnt use the standard logic. Is this just legacy?

Don't remember 100%, but it had something to do with the dock panel suite having their own layout serialization requiring a stream / string / file that is a complete XML, and the intention to keep all editor user data in one file. For some reason it wasn't possible to just read the docking stuff from an XML node within a bigger XML, it had to be root - so the code basically concatenates two XMLs in one file. Not great, but probably best not to touch as part of this PR.

The file itself is definitely not legacy though, all the data in there is still very much in use, and it's also where custom and default editor plugins leave their user settings. It's just the way it is handled / structured that's somewhat ugly.

I also saw there is also a EditorUserData.xml file but decided not to put it in here since its userdata.

Good point! We didn't have a file for that until now, was a matter of time when one would materialize.

That being said, I'm really not sure we should allow to change source or import paths. I don't see a really compelling use case for it, and it is also in line with expectations for the software to just define the way it is. By doing that, we would also provide the reliability for third parties to write clever scripts and additional tooling that can assume where those folders are, because it's the same for every Duality project. I think there's value in having constants here.

Or, from a different angle: It's often the easier path to add more settings, because it leaves the mental load for the decision on the user side - for it to make sense, there has to be a payoff. If the added choice ultimately doesn't matter to the user, having it is a downgrade, because you get the mental load without gaining anything.

Anyway! You decide - read it as subjective recommendation, not a formal change request, and I may have missed something here as well that you already accounted for. (Maybe check back with @SirePi as well on this?)

Barsonax commented 4 years ago

The file itself is definitely not legacy though, all the data in there is still very much in use, and it's also where custom and default editor plugins leave their user settings. It's just the way it is handled / structured that's somewhat ugly.

Ok, was just wondering whether this was some old way of doing things.

That being said, I'm really not sure we should allow to change source or import paths. I don't see a really compelling use case for it, and it is also in line with expectations for the software to just define the way it is. By doing that, we would also provide the reliability for third parties to write clever scripts and additional tooling that can assume where those folders are, because it's the same for every Duality project. I think there's value in having constants here.

The reason I started thinking about this was this (also some other leftover folders from previous itterations :P): image

Due to the 'better' path structure that we are using with the new template the source folder now ends up in the build folder because duality now creates it in the parent directory. Now I could just move everything 1 folder down to workaround this but then I started thinking why not make it configurable so that users themselves can change the path if that better fits their project.

Anyway! You decide - read it as subjective recommendation, not a formal change request, and I may have missed something here as well that you already accounted for. (Maybe check back with @SirePi as well on this?)

Also curious about your opinion about this @SirePi

ilexp commented 4 years ago

Due to the 'better' path structure that we are using with the new template the source folder now ends up in the build folder because duality now creates it in the parent directory. Now I could just move everything 1 folder down to workaround this but then I started thinking why not make it configurable so that users themselves can change the path if that better fits their project.

Ouch 😄 Yeah, I get it. That might be a good reason to move Import into the Duality folder though, and maybe even Source? Depending on an "upwards path" / .. could be inviting problems - if not for the dev, the after the game is one some users machine and they run the bundled editor that came with it as a modding / level editor tool.

Not sure, maybe revisit this with the new points / use cases in mind?

Barsonax commented 4 years ago

Ouch 😄 Yeah, I get it. That might be a good reason to move Import into the Duality folder though, and maybe even Source? Depending on an "upwards path" / .. could be inviting problems - if not for the dev, the after the game is one some users machine and they run the bundled editor that came with it as a modding / level editor tool.

Thats why I wanted to make these paths configurable as that will solve this problem and won't dictate a folder structure on the user. For me atleast I want to keep everything organised in my projects and dualities hardcoded paths can get in the way here. Time to sleep over this :P

ilexp commented 4 years ago

Thats why I wanted to make these paths configurable as that will solve this problem and won't dictate a folder structure on the user. For me atleast I want to keep everything organised in my projects and dualities hardcoded paths can get in the way here.

It doesn't really solve the issue in general, it just moves it over to the user to solve it themselves, because the default setting we provide sometimes works and sometimes doesn't. Of course, if there is simply no sensible default that works for all or most cases, then a user setting really is the solution - but I don't think that's necessarily the case here. I mean, Duality v3 doesn't have the issue right now, and as far as I understand it, even with the new distribution mechanism, there's no technical reason not to keep a self-contained folder structure as-is, right? If that's the case then we'd already have the proof that there's at least one way that solves it without user configuration, which we could expand on.

Barsonax commented 4 years ago

It doesn't really solve the issue in general, it just moves it over to the user to solve it themselves, because the default setting we provide sometimes works and sometimes doesn't. Of course, if there is simply no sensible default that works for all or most cases, then a user setting really is the solution - but I don't think that's necessarily the case here.

But with this change you can now change the default if so desired instead of having to compile duality yourself to change it. So there is a sensible but overridable default.

Duality v3 doesn't have the issue right now, and as far as I understand it, even with the new distribution mechanism, there's no technical reason not to keep a self-contained folder structure as-is, right?

Well technically you could make it work but its not that pretty.

Duality v3 created the solution for you, with duality v4 this no longer happens as this is not possible anymore since you first need the solution to be created before you can get the editor. Makes sense to make this path configurable now as the situation changes from 'Duality managing the solution' to 'Duality integrating with a already existing solution'. In v3 the sourcecodepath simply did much more than in v4 where its only used to open up the source code. Besides having to go through several folders just to get to the solution file is kinda meh.

Duality still creates the folder for the Source path but I think we can remove that part as well. Theres already a exist check in SourceCodeSolutionFilePath so it will print out a error message that it cannot find the solution file in path x.

EDIT: its also used in a filesystem watcher which will throw if the path does not exist so better keep this in.

The import path (media folder) is more like a internal path though so it would make sense to not make this configurable so thinking of reverting that part of this change.

EDIT: reverted the import path change so its now a readonly static string again.

SirePi commented 4 years ago

For me, I understand the point of changing the current structure, especially in light of the new way of creating a project that has been done for v4. Maybe I will personally never move the source/import/assets folders around, as I've been used to the usual way for many years now, but if it's just an extra possibility, why not

ilexp commented 4 years ago

Okay, since you already reverted some parts that I still thought we were in the middle of discussing, I think I need to clarify my position a bit 😄 It's not my intention to make you guys keep this stuff the way it was, and I think restructuring the folders can be a good idea in the wake of the new distribution mechanism. You're right that it shifts focus from the editor towards the distributing VS solution file and .NET tooling, so it would make sense to address this. The part where I'm asking critical questions is basically just me probing use cases and scenarios to make sure it's 100% nice and neat for the user in various situations.

I have to admit, I got lost a bit where we are right now on the design and this PR, so I'll just list a few cases and guidelines that I think we should take into account. Can you summarize the structure this PR currently implements, with all changes applied, and where we stand with regard to the points below?

With v3, the applicable (4/5) cases above work as expected, without the need for the user to adjust anything. I know I know, it was a different situation, but that aside, I think it should still be possible to achieve the same "it just works" use case coverage with whatever new folder structure we come up for v4. Is that currently the case? If not, how do we get there?

(Sorry in case I'm asking questions with obvious answers - lack of time makes me skim some parts of the PR that I should otherwise read into.)

Barsonax commented 4 years ago

The part where I'm asking critical questions is basically just me probing use cases and scenarios to make sure it's 100% nice and neat for the user in various situations.

Thats good, thats why we need you here to challenge our ideas :). So on to the scenarios:

Dev downloads .zip file, extracts it to a new folder, and builds the contained solution. Are all related files in the folder (or subfolders of it) they extracted it into?

Dev creates a new project using .NET CLI tooling in a new folder. Are all related files in the folder (or subfolders of it) that they targeted?

These scenarios will result in the same output, minus the fact that the .NET CLI tooling can adjust some names with replacement parameters. Everything will end up in the folder you extract to and nothing will appear outside that folder.

Dev releases a game without source code, but with the editor. User opens the editor on their machine and imports resources, builds levels. Are all related files in the game folder (or subfolders of it), e.g. the one that contains launcher and editor?

Everything thats needed for duality to run is in the output folder of the build. So everything will work except opening the source code (which will give you a error that it cannot find the solution file). The only issue I see here is that the EditorAppData still has the custom path to the source folder and there is some logic that creates that source folder in the duality editor. In case of the template that means it will create a folder named Source in the parent folder. A solution would be to simply not create this folder automatically anymore and add a check around the filewatcher so that it won't crash if it does not exist.

Dev builds Duality from source, copies it into a new folder to create a new project with the custom build. Are all related files in the folder (or subfolders of it) that they targeted?

Dev builds Duality from source for maintenance / PR reasons, then goes testing while debugging. Are all related files in the Output folder with none escaping?

In these cases it will use the default values so everything will end up in the same folder including the source folder. Basically the same as in v3.

ilexp commented 4 years ago

Great, sounds like we're good to go in 4/5 cases. What I don't get though is this:

In case of the template that means it will create a folder named Source in the parent folder.

If in all other cases all files are contained within the project folder hierarchy, why does the Source folder now appear outside in that one case..? I'm probably missing something here. Can you give me a concrete example?

And to expand on and clarify the use case:

The only issue I see here is that the EditorAppData still has the custom path to the source folder and there is some logic that creates that source folder in the duality editor.

That's actually a valid part of the case! People who mod a game they got can very well just add a second game plugin with their own logic to extend it further. At least if the Duality editor was still responsible for creating the game plugin solution and projects, this should continue to work.

Since that's not the case anymore, then at least (a) it shouldn't crash like you said and (b) should be open to start working again if they create a solution and plugin project themselves. Maybe it could even (c) provide an in-editor shortcut to create a plugin-only solution without any management of Duality binaries?

Either way, for the modding case, it would actually be kinda nice if it could be in a subfolder, since the project is already there in binary form, and they just want to extend it.

still has the custom path to the source folder

This part is ugly though, both for privacy reasons and because it breaks stuff.

Maybe we can get rid of both the source setting and the issue if we change folder structure to do something like the following:

[Project Folder]
|
+- [Project].sln
|
+- DualityEditor.exe
+- Data
+- Plugins
+- all the other project files and binaries
|
+- Import
|
+- Source
   +- Launchers
      +- GameLauncher
      +- GameEditor
   +- Plugins
      +- GamePlugin

or even

[Project Folder]
|
+- [Project].sln
|
+- DualityEditor.exe
+- Data
+- Plugins
+- all the other project files and binaries
|
+- Source
   +- Import
   +- Launchers
      +- GameLauncher
      +- GameEditor
   +- Plugins
      +- GamePlugin

That way, we guarantee that (a) everything is in the project folder and we won't ever need to access a parent, and (b) we don't even need the custom paths for stuff to work, regardless of whether there's a setting or not. All the source stuff is still in a proper subfolder, only the solution file itself moved to the root, which is now the project folder again.

add a check around the filewatcher so that it won't crash if it does not exist.

That sounds like a good idea in any case 👍

Barsonax commented 4 years ago

If in all other cases all files are contained within the project folder hierarchy, why does the Source folder now appear outside in that one case..? I'm probably missing something here. Can you give me a concrete example?

The publish logic currently only copies files from the folder that duality editor is currently in. It then puts all these files at the same folder in the zip.

Another thing I noticed about the publish logic is that it has a separate list of hardcoded paths which are not yet updated. This has to be fixed for v4 regardless of if we change the folder structure or not.

That way, we guarantee that (a) everything is in the project folder and we won't ever need to access a parent, and (b) we don't even need the custom paths for stuff to work, regardless of whether there's a setting or not. All the source stuff is still in a proper subfolder, only the solution file itself moved to the root, which is now the project folder again.

It does mix source and buildoutput in the same folder though which I find quite ugly. It also complicates the .gitignore file as you cannot just exclude all .dll files from the duality folder for instance. Might even lead to issues if the build copies files to the source folder (though thats a unlikely scenario).

Aside from the above if in the future we want to do things like multitargeting to target multiple platforms we have to separate the source (and more) from duality anyway so this would be a good step towards that.

This part is ugly though, both for privacy reasons and because it breaks stuff.

Another idea what if instead of the editorappdata setting we had a .dualityprojectroot file somewhere in a folder which defines what is the root of the project? If there is none it defaults to the folder that dualityeditor is currently running in. That way the publish logic can keep the folder structure intact and theres no need for any (possibly absolute) paths to be defined that might leak personal information.

EDIT: While I do think making this possible is a good improvement iam also starting to see this is taking more time than anticipated. I think iam gonna go with the folder structure you proposed:

[Project Folder]
|
+- [Project].sln
|
+- DualityEditor.exe
+- Data
+- Plugins
+- all the other project files and binaries
|
+- Import
|
+- Source
   +- Launchers
      +- GameLauncher
      +- GameEditor
   +- Plugins
      +- GamePlugin

I will move and further discussion/changes to a future issue so we don't have to wait on this anymore to release v4 and have plenty of time to review all the edge cases or come up with even better ideas. See #827 for the revised PR and #828 for the issues to continue discussing this.