Ticket #1301 (closed enhancement: wontfix)

Opened 14 months ago

Last modified 9 months ago

[PATCH] Priest Praying

Reported by: trumbun Owned by: clifton
Priority: Nice to Have Milestone:
Component: UI & Simulation Keywords:
Cc:

Description

Attachments

prayingpatch.patch (9.1 KB) - added by trumbun 13 months ago.
Updates that I done to improve the function and the comment by leper
pray.png (1.4 KB) - added by trumbun 13 months ago.
Praying icon during testing placed it in \binaries\data\mods\public\art\textures\ui\session\icons

Change History

comment:1 Changed 14 months ago by leper

  • Summary changed from [Patch] Priest Praying to [PATCH] Priest Praying

If you submit a patch and want it reviewed please add the review keyword.

I don't really know if we want this feature (you should better discuss this in our forum), but I'll give you some feedback on your code.

pray.png: If you add binary files please upload them too as they are not included in patch files.
Input.js:
lines 326-328: You should remove one tab before the case statement and add one tab in front of the break.
line 400: Remove the tab after the semi-colon.
lines 1263-1265: Remove the pretty useless comment and indent the break.

Pray.js:
TryInstantGather (probably influenced by the function of the same name in ResourceGatherer.js) should be renamed to something more appropriate like PerformPray or just Pray.
Remove the pretty useless comment on top and don't return anything as you don't even check for the return value.
Use [] instead of new Array as noted in our coding conventions.
The comments are not really helpful as they just tell you what the code does and not why.
This function could also use some refactoring.

UnitAI.js:
346: Obvious comment is obvious... remove that one.
Move the SelectAnimation call next to the commented one to make sure it is replace once there is an animation in place.
Elaborate why we should ignore "Attacked" events (that comment isn't really helpful and just because I can figure out why we should rather die than stop praying doesn't mean that anyone reading the code can figure it out).
Is the MoveCompleted() needed?
line 2430: Remove the warning (yes I know that can be useful to figure out if some code gets called but it shouldn't be included in a patch)
line 2431: Is there a reason why you use ReplaceOrder instead of AddOrder?

Commands.js:
Remove the warning and the commented code.
Don't indent GetFormationUnitAIs. It should have the same indentation as the var enitities = ... line.

Thanks for your patch.

Last edited 14 months ago by leper (previous) (diff)

Changed 13 months ago by trumbun

Updates that I done to improve the function and the comment by leper

Changed 13 months ago by trumbun

Praying icon during testing placed it in \binaries\data\mods\public\art\textures\ui\session\icons

comment:2 Changed 10 months ago by Spahbod

I know that we don't want priest praying, but maybe we can implement the logic into the game for future modders?

comment:3 Changed 10 months ago by historic_bruno

For that case, I almost think we should toss it into a demo or community mod pack. We do plan to have at least one demo mod (beyond "public") in the final release as a demonstration of how modding works.

comment:4 Changed 10 months ago by Spahbod

But as time passes, it would be harder to implement it's logic into the game. We can have it as a non-used feature till release.

comment:5 follow-up: ↓ 6 Changed 10 months ago by fcxSanya

I don't like the idea of adding unused features into the codebase just in case. Each addition make code harder to maintain. Patch is useful by itself, if someone will need this functionality he can use it at least for reference, most likely it would be possible to apply it after little updating even after long amount of time.

comment:6 in reply to: ↑ 5 Changed 10 months ago by historic_bruno

Replying to fcxSanya:

I don't like the idea of adding unused features into the codebase just in case. Each addition make code harder to maintain. Patch is useful by itself, if someone will need this functionality he can use it at least for reference, most likely it would be possible to apply it after little updating even after long amount of time.

Agreed, at this point I think the simulation architecture is basically set in stone so even if the patch doesn't apply cleanly in say, 6 months (the UI part will certainly change), it would still be useful as a concept.

comment:7 follow-up: ↓ 8 Changed 9 months ago by trumbun

Can my patch be applied after the normal installation? If, yes how? I am using ubuntu 12.04. Thanks

comment:8 in reply to: ↑ 7 Changed 9 months ago by historic_bruno

Replying to trumbun:

Can my patch be applied after the normal installation? If, yes how? I am using ubuntu 12.04. Thanks

You would add the new/modified files to your own custom mod, see the Modding Guide for some info on that. The new mod files should replace the originals. It will be slightly painful to maintain compatibility as time goes by, but that will improve as we finish the game (the data won't change as often).

comment:9 Changed 9 months ago by historic_bruno

  • Status changed from new to closed
  • Resolution set to wontfix
  • Milestone Backlog deleted
Note: See TracTickets for help on using tickets.