HomeWildfire Games

Write mod data correctly. Fixes #1940.
Concern RaisedrP13472

Description

Write mod data correctly. Fixes #1940.

Details

Auditors
elexis
Committed
leperJun 11 2013, 10:50 PM
Parents
rP13471: Updated some building tooltips.
Branches
Unknown
Tags
Unknown

Event Timeline

elexis raised a concern with this commit.Apr 3 2019, 7:01 AM
elexis added a subscriber: elexis.
  1. The comment is wrong https://wildfiregames.com/forum/index.php?/topic/25579-cannot-play-custom-maps-please-help/&do=findComment&comment=371878
  1. We may want to reconsider whether the feature design is ideal. From https://trac.wildfiregames.com/ticket/1940#comment:3

I don't really like the inconsistency in the dev and non-dev cases, in terms of mounting the read-only and user-writable mods

This surprise has resulted in problems before, for example for mod-version checks that have to hardcode a "user" exclusion.

  1. The special "user" mod behavior is not documented anywhere other than being buried in the middle of a C++ code file.
  1. One may reconsider whether it is actually necessary to write to a "user/" mod folder instead of a "public/" folder, i.e. whether adding a "user" mod was necessary to begin with and not adding a solution for the right problem in the wrong level of the code.
/ps/trunk/source/ps/GameSetup/GameSetup.cpp
431

^

This commit now has outstanding concerns.Apr 3 2019, 7:01 AM
smiley added a subscriber: smiley.Apr 7 2019, 5:51 PM
smiley added inline comments.
/ps/trunk/source/ps/GameSetup/GameSetup.cpp
431

How is the comment wrong?

smiley added inline comments.Apr 7 2019, 6:14 PM
/ps/trunk/source/ps/GameSetup/GameSetup.cpp
431

Oh..that one specific word. Yes, it is incorrect. Incorrect like a typo in my opinion. If you are only reading the comment and not the code relating to it, you are doing something wrong.
The purpose of the dev check is to have maps saved in a dev copy to end up in the source. So, what is the point of having a "noUserMod" option in a dev copy.

elexis added inline comments.Apr 7 2019, 8:05 PM
/ps/trunk/source/ps/GameSetup/GameSetup.cpp
431

If you are only reading the comment and not the code relating to it, you are doing something wrong.

It's precisely because I read the code relating to it that I noticed the comment to be slightly incorrect and lacking explanation as to why the code is doing what it does. The purpose of comments is to (1) summarize the code behavior and (2) state the reasons behind the code behavior.

Oh..that one specific word. Yes, it is incorrect. Incorrect like a typo in my opinion.

The first of these four points typo-grade indeed.
I had hoped that the forum posts communicated some of my thoughts, I don't mind clarifying or repeating if it helps resolve the case.

The purpose of the dev check is to have maps saved in a dev copy to end up in the source.

That is what the code does, but not a reason as to why it should do that.

From the two trac tickets, it seems this was only done to address one single error stack:

Reproduce:

  1. Open map
  2. Edit map
  3. Save map
  4. Close map
  5. Open map

Expected Result:

  1. Notice previous edits

Actual Result:

  1. Previous edits absent because Atlas loads from the wrong path

So that was the historic stack trace that needed to be fixed and _not_ any further design decision, at least not printed anywhere, not in the code, not in the two tickets. The only thing we find in the ticket is someone claiming that he thinks that precisely the different behavior for release-code and dev code is more bothersome than desirable and is recommended for revision.

There are a number of solutions, they should all be evaluated without prejudice before rushing one solution, or we should at least be able to later reevaluate instead of claiming a temporary fix to be the obvious best solution.

I mentioned the alternative on the forums already: .local/share/0ad/mods/public/ instead of .local/share/0ad/mods/user/,
where public is the topmost mod that launched the file into the VFS, or the topmost mod if the file does not exist.

The solution would have the following advantages over the existing:

  • fixing the Atlas and equivalent problems equally
  • not inserting an alternative C++ code path to launch a mod
  • not defining Mod and VFS logic in GameSetup.cpp to fix the case
  • not removing the workaround by adding more workarounds elsewhere to account for the first workaround (not having to spread and keep in sync N mod version checks across the JS and C++ codebase)
  • not adding a mod that is not defined by mod.json (consistency)
  • not adding a mod (least complexity)
  • not missing documentation on a magic modname in the Mod C++ classes.
  • if done right, not missing documentation in the VFS C++ classes on behavior of writing files.
  • not missing documentation on a magic modname in the wiki modding pages
  • keeping all content relating to one mod in the same write-folder of that mod (consistency)
  • having the program only refer to mods that were defined by mod folders (simplicity)
  • making the release code not behave differently from development code (consistency)
  • if not refuted, it would implement the concerns of the reviewer and auditor.
  • might cost more bananas to implement the code though.
smiley added inline comments.Apr 7 2019, 8:12 PM
/ps/trunk/source/ps/GameSetup/GameSetup.cpp
431

(To clarify: “you” was meant in a collective context. Not specific to anyone.)
Apart from that, I really don’t have much to say.