Opened 12 years ago

Closed 12 years ago

#1301 closed enhancement (wontfix)

[PATCH] Priest Praying

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

Description

Attachments (2)

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

Download all attachments as: .zip

Change History (11)

comment:1 by leper, 12 years ago

Summary: [Patch] Priest Praying[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 12 years ago by leper (previous) (diff)

by Clifton, 12 years ago

Attachment: prayingpatch.patch added

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

by Clifton, 12 years ago

Attachment: pray.png added

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

comment:2 by O.Davoodi, 12 years ago

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

comment:3 by historic_bruno, 12 years ago

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 by O.Davoodi, 12 years ago

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 by fcxSanya, 12 years ago

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.

in reply to:  5 comment:6 by historic_bruno, 12 years ago

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 by Clifton, 12 years ago

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

in reply to:  7 comment:8 by historic_bruno, 12 years ago

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 by historic_bruno, 12 years ago

Milestone: Backlog
Resolution: wontfix
Status: newclosed
Note: See TracTickets for help on using tickets.