Merging Mods, Collaborating

Discuss Daggerfall Unity and Daggerfall Tools for Unity.
Post Reply
User avatar
Interkarma
Posts: 3591
Joined: Sun Mar 22, 2015 1:51 am

Merging Mods, Collaborating

Post by Interkarma » Wed Oct 21, 2015 10:49 pm

Starting from the next push, I will begin rolling some of the outstanding community enhancements into the core library. This includes work by Nystul, LypyL, and Uncanny_Valley.

What this means is we are all using the same code base. Changes to your code will be pulled into the work-in-progress branch, which in turn will eventually be rolled into the stable master branch.

I will add the latest versions I have of your work into the work-in-progress branch. These will be positioned in the Assets/Daggerfall Unity/Game/Addons folder. I will add enhancements one at a time and we can focus on any fixes needed for them as required.

Once your enhancement is checked in by myself, below will be the collaborative workflow moving forwards.
  1. In your github account, fork my work-in-progress build.
  2. When you are ready to make changes to your code, create a new branch from your fork. This allows you to experiment without affecting anything else. You can switch between branches in your git client.
  3. Make any changes you need to your code. If changes are required to elsewhere in the library, make those as well. Don't forget to put your name in the // contributors comment line near the top.
  4. Once you're happy with the changes, merge them back into your fork of my work-in-progress branch.
  5. Send me a pull request of your changes. I will review and if no problems will accept pull request back into the work-in-progress branch.
With your enhancements becoming part of the main repository, certain extra responsibilities are expected.
  • Your code must build from the Unity Editor with zero errors and zero warnings. Even warnings like "variable foo was assigned but never used" must be resolved.
  • Don't change anything unrelated to your required code changes. If you need to experiment, that's OK, but do it in a safe branch or offline copy (or revert files back to how they were before checking in).
  • Use the standard code header in each file, naming yourself as the original author for your files, or as a contributor if you change another person's code.
  • It's OK to have your own code/formatting style, but when editing another person's code please try to keep the style consistent with theirs. This applies to me as well if I need to edit your code.
  • If your enhancement requires input, do not use Input.KeyDown or similar. Please create an action in the InputManager and assign a default key to that action. The InputManager is our central point for all game input moving forward. For non-game input (e.g. a debug console), it's OK to use the usual Unity Input namespace directly.
I know that seems like a lot to take in (it kind of is), but it's going to ensure we're all on the same page, working together as a team and working on the same code. This means I won't be breaking your stuff as often and you'll have less work to do anytime you create an update.

For new mod creators that aren't in the Game/Addons folder yet, please just send me a .assetpackage of your work to start with, keeping in mind the guidelines above. I'll do a review, let you know my comments, and hopefully can get you into the core code before long.

User avatar
Nystul
Posts: 1182
Joined: Mon Mar 23, 2015 8:31 am

Re: Merging Mods, Collaborating

Post by Nystul » Thu Oct 22, 2015 12:04 pm

Thanks for all your efforts and farsight!
I will try my best to meet all those requirements! ;)

update:
managed to fix the problems of my ReflectionMod in version 0.31 with the work-in-progress branch - see my according thread in the creator's corner: (viewtopic.php?f=14&t=122&p=1211#p1211)
Dependent on which version you will check in, I will make sure to make all necessary changes anyway as soon as something is checked in.

User avatar
Nystul
Posts: 1182
Joined: Mon Mar 23, 2015 8:31 am

Re: Merging Mods, Collaborating

Post by Nystul » Mon Oct 26, 2015 11:51 am

I have managed to update my mods so they should work with daggerfall-unity.
how do we proceed? do you want me to add them in a fork on my side and make a push request?

User avatar
Interkarma
Posts: 3591
Joined: Sun Mar 22, 2015 1:51 am

Re: Merging Mods, Collaborating

Post by Interkarma » Mon Oct 26, 2015 12:07 pm

Yes please. Fork the work-in-progress branch, push your changes, then create a pull request for me. I will review the changes and accept the pull request.

One more thing that Lypyl just encountered. You should go to Edit > Project Settings > Editor and ensure the following settings are:

Version Control Mode - Visible Meta Files
Asset Serialization Mode - Mixed

I look forward to getting seeing it in action in the live code! :)

User avatar
Nystul
Posts: 1182
Joined: Mon Mar 23, 2015 8:31 am

Re: Merging Mods, Collaborating

Post by Nystul » Mon Oct 26, 2015 3:58 pm

so first time using git - hard fail on my side - forgot to add all the new files - so first pull request was only the changes to MaterialReader...
second pull request issued - hopefully I didn't ruin it :/

update:
hilarious commit comment fail: wasn't the best idea to write a comment which lists all changes - first list item is now the commit message title

CyberWilhelm
Posts: 17
Joined: Mon Oct 19, 2015 6:57 pm

Re: Merging Mods, Collaborating

Post by CyberWilhelm » Mon Oct 26, 2015 4:44 pm

I'm also completely new to Git, though I've been interested in learning it for a while (mostly Subversion experience, here). Anyway, in case it's useful, Linus Torvalds (creator of Git and Linux) has some comments about what makes a well-formed Git log message. It's worth paying attention to because I believe most Git tools expect comments in this form.

https://gist.github.com/matthewhudson/1475276

Code: Select all

A good commit message looks like this:

	Header line: explaining the commit in one line

	Body of commit message is a few lines of text, explaining things
	in more detail, possibly giving some background about the issue
	being fixed, etc etc.

	The body of the commit message can be several paragraphs, and
	please do proper word-wrap and keep columns shorter than about
	74 characters or so. That way "git log" will show things
	nicely even when it's indented.

	Reported-by: whoever-reported-it
	Signed-off-by: Your Name <youremail@yourhost.com>

where that header line really should be meaningful, and really should be
just one line.  That header line is what is shown by tools like gitk and
shortlog, and should summarize the change in one readable line of text,
independently of the longer explanation.
(Side note: Is there a preferred way to quote sources from outside of the forum? The quote tag seems like it's intended for just quoting other local posts.)

Anyway, it'll be a while before I can even think about contributing code, but thought I'd share this. :)

CyberWilhelm
Posts: 17
Joined: Mon Oct 19, 2015 6:57 pm

Re: Merging Mods, Collaborating

Post by CyberWilhelm » Mon Oct 26, 2015 5:34 pm

Interkarma wrote:One more thing that Lypyl just encountered. You should go to Edit > Project Settings > Editor and ensure the following settings are:

Version Control Mode - Visible Meta Files
Asset Serialization Mode - Mixed
Just to double-check, it looks like we all need to set up our own Unity project for the game? That's what I ended up doing when testing the full-screen fix with my Rift DK2 the other day. It worked for my quick test, but I want to make sure I'm not missing something in case I get time to work on something larger.

User avatar
Interkarma
Posts: 3591
Joined: Sun Mar 22, 2015 1:51 am

Re: Merging Mods, Collaborating

Post by Interkarma » Mon Oct 26, 2015 8:46 pm

Nystul wrote:so first time using git - hard fail on my side - forgot to add all the new files - so first pull request was only the changes to MaterialReader...
second pull request issued - hopefully I didn't ruin it :/

update:
hilarious commit comment fail: wasn't the best idea to write a comment which lists all changes - first list item is now the commit message title
Cheers. :) I've reviewed the code and it looks good. I just made one comment around how the terrain sampler is being set. That line of code is basically injecting your terrain sampler whether addon is supposed to be active or not.

For now, the quickest way to solve this is simply to check DaggerfallUnity.Settings.Nystul_* flags during Awake() and Start(). My game startup code can then disable addon objects in the scene if user has indicated they should not be active.

Let me know if you'd prefer me to make that small change after accepting pull request. :)

User avatar
Interkarma
Posts: 3591
Joined: Sun Mar 22, 2015 1:51 am

Re: Merging Mods, Collaborating

Post by Interkarma » Mon Oct 26, 2015 8:53 pm

CyberWilhelm wrote:
Interkarma wrote:One more thing that Lypyl just encountered. You should go to Edit > Project Settings > Editor and ensure the following settings are:

Version Control Mode - Visible Meta Files
Asset Serialization Mode - Mixed
Just to double-check, it looks like we all need to set up our own Unity project for the game? That's what I ended up doing when testing the full-screen fix with my Rift DK2 the other day. It worked for my quick test, but I want to make sure I'm not missing something in case I get time to work on something larger.
Unfortunately yes. I've made the decision to not include project settings in the repo as there are a few issues around mixing projects between Pro and Personal. It's one thing for a team all licensed on Pro to share a project, but for a FOS project like this it gets more complicated. That's why I'm just pushing .cs and other base files to git.

The good news is things are generally engineered to require no project config. With the exception of setting up layers for addons, everything will pretty much "just work" in a freshly imported project by opening main scene. At some point, I'll need to write a bullet-list for contributors though.

User avatar
Nystul
Posts: 1182
Joined: Mon Mar 23, 2015 8:31 am

Re: Merging Mods, Collaborating

Post by Nystul » Mon Oct 26, 2015 10:16 pm

Interkarma wrote: Let me know if you'd prefer me to make that small change after accepting pull request. :)
of course I am happy with everything that works and that makes our life easier ;)
update: since the changes were small, I just did it and did another pull request (I start to like git)
Interkarma wrote: That line of code is basically injecting your terrain sampler whether addon is supposed to be active or not.
Yeah I had this already before, but commented it out while fixing all the problems - so this is easy to do again ;)


one more question from my side:
Both my mods use external files, all files are in a corresponding Resources folder inside the mod subfolder. png-files are loaded with Resources.Load which seems to load the files always correctly.
But there are other files where Resources.Load didn't work (e.g. the ini file for the texture injection config):
I am not sure if the relative filepath is ok in terms of building the final executable - I use these relative paths:
ReflectionsMod:

Code: Select all

const string filepathConfigInjectionTextures = "Assets/daggerfall-unity/Game/Addons/ReflectionsMod/Resources
/configInjectionTextures.ini";
IncreasedTerrainDistanceMod:

Code: Select all

        const string filepathInTreeCoverageMap = "./Assets/daggerfall-unity/Game/Addons/IncreasedTerrainDistanceMod/Resources/mapTreeCoverage_in.bin";

        const string filepathOutTreeCoverageMap = "./Assets/daggerfall-unity/Game/Addons/IncreasedTerrainDistanceMod/Resources/mapTreeCoverage_out.bin";

        const string filepathMapLocationRangeX = "./Assets/daggerfall-unity/Game/Addons/IncreasedTerrainDistanceMod/Resources/mapLocationRangeX.bin";
        const string filepathMapLocationRangeY = "./Assets/daggerfall-unity/Game/Addons/IncreasedTerrainDistanceMod/Resources/mapLocationRangeY.bin";
just want to find out if this will be a problem

btw, there is a shader warning I am not able to get rid of easily:
Shader warning in 'Daggerfall/IncreasedTerrainTilemap': gradient-based operations must be moved out of flow control to prevent divergence. Performance may improve by using a non-gradient operation at line 163 (on d3d11)
it is caused by a tex2Dgrad command that is sampling from the different atlas textures inside if statements dependent on the climate for a tile. I do not agree with the warning though that moving it out of the if statement will improve performance since I would have to sample all the different atlas textures anyway :D
I will rethink the strategy if it is important to change it ;)
If it is ok I will leave it unchanged for the moment

Post Reply