Skip to content
This repository has been archived by the owner on Dec 4, 2020. It is now read-only.

Latest commit

 

History

History
652 lines (524 loc) · 14.6 KB

CONTRIBUTING.md

File metadata and controls

652 lines (524 loc) · 14.6 KB
Raw

NOTICE: AS OF NOVEMBER 16TH, 2020, PROJECT TOPAZ IS ONLY ACCEPTING COMMUNITY PULL REQUESTS FOR CRITICAL CRASH FIXES, EXPLOIT FIXES, AND VERSION UPDATES. ANY PULL REQUEST FOR ANOTHER PURPOSE -- SUCH AS NEW CONTENT, REFACTORING, OR NON-VITAL FIXES -- WILL BE CLOSED WITHOUT COMMENT OR REVIEW.

Issue Report Contributions:

  • If an issue involves incorrect NPCs or text, please include your client version (type /ver in game)
  • Unimplemented feature requests must be retail behavior, and adequetly cover everything about that feature which is missing.

Pull Request Contributions:

By submitting a pull request to Project Topaz, you agree to our Limited Contributor License Agreement

All contributions must be done through pull requests to the Topaz repository. We don't take fixes from Discord to apply ourselves. If you need help with making a pull request, there is a GitHub guide on how to do so. If you still need help after consulting the guide, you can ask for help in Discord and we will be happy to help you.

We prefer submitting early and often, over monolithic and once. If you're implementing a complex feature, please try to submit PRs as you get each smaller functional aspect working (use your best judgment on what counts as a useful PR). This way we can help make sure you're on the right track before you sink a lot of time into implementations we might want done in a different way.

Please try to leave your PR alone after submission, unless it's to fix bugs you've noticed, or if we've requested changes. If you're still pushing commits after opening the PR, it makes it hard for reviewers to know when you're "finished" and if it's "safe" to begin their reviews. If you do want to push early for reviews of your in-progress work, you can open your PR as a "draft".

After a pull request is made, if a staff member leaves feedback for you to change, you must either fix or address it for your pull request to be merged.

If you do not fill the checkboxes confirming that you agree to Project Topaz's Limited Contributor License Agreement and that you've tested your code - your PR will not be reviewed.

Workflow Guide

  • It is always better to come into Discord and ask a question instead of investing a lot of time in work that we're going to ask your to rewrite or split up.
  • Cite your sources for things that aren't obvious. This can be comments in your code, or your commit messages. Pull Request descriptions and comments will get lost over time, information in the repo lasts forever.
  • If you're commiting work on someone else's behalf, use git's --author argument so they get the credit they deserve.
  • Make your commit messages meaningful, or amend/rebase once you're ready to push.

Style Guide

Code Editor Configuration

Much of this can be automated.

We highly recommend editorconfig, which most code editors have either a plugin or native support for.

  • Visual Studio Plugin
  • Notepad++
    • As the plugin manager is usually installed by default*, the easy way is to use that: Launch Notepad++, click on the Plugins menu, then Plugin Manager -> Show Plugin Manager. In the Available tab, find EditorConfig in the list, check the checkbox and click on the Install button.
      • *64bit may require manual installation.
  • Sublime: Install EditorConfig with Package Control and restart Sublime.
  • Vim

Clang-Format is also an option for C++

General code guidlines (all languages):

  • Your code should strive to be obvious and readable by the casual observer. You aren't going to be the only person who reads/debugs your code.
  • Unix (LF) line ends at the end of every file. GitHub will tell you if you don't have one by putting a ⛔ symbol at the end of your file.
  • Try not to exceed 120 chars width. Exceptions will occur, but try.
  • 4 space indent, do not use tabs for alignment.
  • Trim trailing whitespace.
  • Space after starting comments (-- Comment and // Comment)
  • If you agree with staff and/or your reviewers that some work in your pull request can be left as "to-do", make new issues on GitHub for your new TODO items and put the ID alongside the comment. The comment should also be sufficiently descriptive of what the missing work is, and why it was left out. eg. // TODO: A Boy's Dream - PLD AF Quest 2 - Cannot be completed until fishing is implemented (GitHub Issue #12345)

C++

We keep a .clang-format file in the root of the repo, but accept it can be difficult to set up for use on just your changes, as opposed to entire files that you're working with that might have legacy styling you don't want to mess with.

Here are the points from .clang-format explained:

BasedOnStyle: WebKit

When in doubt, defaulting to WebKit style with Allman braces is seemingly a safe option.

AccessModifierOffset: -4

// Correct ✔️ 
class Classname
{
public:
    Classname();

private:
    int member;
}

// Wrong ❌
class Classname
{
    public:
    Classname();

    private:
    int member;
}

AllowShortFunctionsOnASingleLine: Empty

// Correct ✔️ 
void f()
{ 
    foo(); 
}

// Wrong ❌
void f() { foo(); }

BreakBeforeBraces: Allman

Braces should almost always be on a new line.

// Correct ✔️ 
if (x == 5)
{
    function();
}

// Wrong ❌
if (x == 5) {
    function();
}

BreakConstructorInitializers: BeforeComma & ConstructorInitializerIndentWidth: 0

// Correct ✔️ 
Constructor(int param0, int param1)
: member0(param0)
, member1(param1)
{
}

// Wrong ❌
Constructor(int param0, int param1)
    : member0(param0), member1(param1){}

CompactNamespaces: 'false'

// Correct ✔️ 
namespace Foo
{
    namespace Bar
    {
    }
}

// Wrong ❌
namespace Foo { namespace Bar {
}}

Cpp11BracedListStyle: 'false'

// Correct ✔️ 
std::vector<int> x{ 1, 2, 3, 4 };

// Wrong ❌
std::vector<int> x{1, 2, 3, 4};

IndentCaseLabels: 'true'

// Correct ✔️ 
switch(x)
{
    case 0:
    {
        break;
    }  
}

// Wrong ❌
switch(x)
{
case 0:
{
    break;
}  
}
  • Note: It doesn't matter if your break; is inside or outside the body of your case statement - as long as it's there (if you indend it to be).

IndentWidth: 4

// Correct ✔️ 
if (func())
{
    reaction();  
}

// Wrong ❌
if (func())
{
  reaction();  
}

KeepEmptyLinesAtTheStartOfBlocks: 'false'

// Correct ✔️ 
void function(int x)
{
    doSomething(x);
}

// Wrong ❌
void function(int x)
{

    doSomething(x);
}

Language: Cpp

Yup.

PointerAlignment: Left

// Correct ✔️ 
void function(CBigType* type);
void function(CBigType& type);

// Wrong ❌
void function(CBigType *type);
void function(CBigType &type);

// Wrong ❌
void function(CBigType * type);
void function(CBigType & type);

SortIncludes: 'true' & SortUsingDeclarations: 'true'

  • Try to keep your include and using statements organised alphabetically, in logical blocks.

SpaceBeforeParens: ControlStatements

// Correct ✔️ 
if (true)
{
    doThing();
}

// Wrong ❌
if(true)
{
    doThing();
}

Standard: Cpp11

// Correct ✔️ 
A<A<int>>

// Wrong ❌
A<A<int> >

UseTab: Never

// Correct ✔️
<4 spaces>

// Wrong ❌
<a tab>

// Wrong ❌
<a half-tab>

Braces Around Statements

readability-braces-around-statements

// Correct ✔️ 
if (x == 5)
{
    function();
}

// Wrong ❌
if (x == 5)
    function();

// Wrong ❌
if (x == 5)
    function(21);
else
{
    function(42);
}

Breaks between consecutive conditional statements

// Correct ✔️ 
if (thisThing())
{

}
else
{

}

if (thatThing())
{

}

// Correct ✔️ 
if (thisThing())
{

}

if (thatThing())
{

}

// Wrong ❌
if (thisThing())
{

}
else
{

}
if (thatThing())
{

}

// Wrong ❌
if (thisThing())
{

}
if (thatThing())
{

}

Casting - static_cast over C-Style

cppcoreguidelines-pro-type-cstyle-cast

// Correct ✔️ 
uint32 param = static_cast<uint32>(input);

// Wrong ❌
uint32 param = (uint32)input;

Don't use static_cast to downcast: Use dynamic_cast instead

cppcoreguidelines-pro-type-static-cast-downcast

// Correct ✔️ 
if (auto PChar = dynamic_cast<CCharEntity*>(baseEntity))
{
    PChar->DoSomething()
}

// Wrong ❌
auto PChar = static_cast<CCharEntity*>(baseEntity)
PChar->DoSomething()
// Wrong ❌

if (auto PChar = static_cast<CCharEntity*>(baseEntity))
{   
    // The cast is forced, so PChar will _always_ be populated here....
    PChar->DoSomething()
}

Arg/Param spacing

// Correct ✔️ 
auto f(0, 1, 2, 3, 4, 5, 6);

// Wrong ❌
auto f(0,1,2,3,4,5,6);

Lambdas

Formatting tools have a famously difficult time with lamdas, here is an example lambda. If you're using them (lambdas, or tools), do your best!

// Correct ✔️ 
auto isEntityAlive = [&](CBigEntity* entity) -> bool
{
    return entity->isAlive;
};

// Correct ✔️ 
static_cast<CCharEntity>(PPlayer)->ForParty([&](CBattleEntity* entity)
{
    entity->doStuff();
});

// Wrong ❌
auto isEntityAlive =
[&](CBigEntity* entity)
    {
        return entity->isAlive;
    };

// Formatting Emergencies ❓
// clang-format off
auto isEntityAlive = [&](CBigEntity* entity) -> bool
{
    return entity->isAlive;
};
// clang-format on

C++ Naming & Misc

  • The STL is your friend, don't be afraid to use it.
  • Be careful with auto, it can mask important type details.
  • Be as const as you reasonably can.
  • UpperCamelCase for namespaced functions and classes.
  • UPPER_SNAKE_CASE for enums (exception for enum classes: style as classes).

Lua

A lot of the styling rules from the C++ guide can and should be applied to Lua code. Here are the important points to remember when styling your Lua:

Local vars are (almost) always preferred

-- Correct ✔️ 
local var = 0

-- Wrong ❌
var = 0

Allman Braces

-- Correct ✔️ 
local table =
{
    content = 1,
}

-- Wrong ❌
local table = {
    content = 1,
}
  • NOTE: The final entry in a multi-line table should have a comma after it.

No parentheses unless needed to clarify order of operations

-- Correct ✔️ 
if condition1 == 1 then
    trigger()
end

-- Correct ✔️ 
if condition1 and (condition2 or condition3) then
    trigger()
end

-- Wrong ❌
if (condition1 == 1) then
    trigger()
end

No semicolons

-- Correct ✔️ 
local x = 42
trigger(42)

-- Wrong ❌
local x = 42;
trigger(42);

-- Wrong ❌
local x = 42; trigger(42);

Formatting Conditional Blocks

-- Short - Correct ✔️
if condition then
    bla()
end

-- Long or many multiple conditions - Correct ✔️
if
    condition1 and
    condition2 or
    not condition3
then
    bla()
end

-- Wrong ❌
if  condition1 then
    bla()
end

-- Wrong ❌
if condition1 and condition2 and
    not condition3 then
    bla()
end

-- Wrong ❌
if condition1 and condition2 and
    not condition3
then
    bla()
end

Placement of logical operators in long blocks

  • not before, and/or after
-- Correct ✔️
if
    condition1 and
    condition2 or
    not condition3
then
    bla()
end

-- Wrong ❌
if
    condition1
    and condition2
    or not condition3
then
    bla()
end

No excess whitespace inside of parentheses or solely for alignment

-- Correct ✔️ 
if condition1 and (condition2 or condition3) then
    trigger()
end

-- Wrong ❌
if condition1 and ( condition2 or condition3 ) then
    trigger()
end

-- Wrong ❌
if
      condition1 and 
    ( condition2 or condition3 )
then
    trigger()
end

Inline tables

THIS IS THE ONE EXCEPTION TO THE GLOBAL NEWLINE-BRACE RULES

-- Correct ✔️ 
tpz.func({
  entry = 1,
  entry = 2,
})

-- Wrong ❌
tpz.func(
    {
        entry = 1,
        entry = 2,
    }
)

Lua Misc & Naming

  • Our lua functions are typically lowerCamelCased, with few exceptions.
  • Make sure you check out scripts/globals/npc_util.lua for useful tools and helpers.
  • If you're going to cache a long table entry into a var with a shorter name, make sure that name still conveys the original meaning.
-- Correct ✔️ 
local copCurrentMission = player:getCurrentMission(tpz.mission.log_id.COP)
local copMissionStatus = player:getCharVar("PromathiaStatus")
local sandyQuests = tpz.quest.id.sandoria

-- Wrong ❌
local currentMission = player:getCurrentMission(tpz.mission.log_id.COP)
local missionStatus = player:getCharVar("PromathiaStatus")
local quests = tpz.quest.id.sandoria

SQL

  • Don't put single quotes around non string fields:

    42,0

    not:

    '42','0'

  • No line breaks in the middle of a statement:

    INSERT INTO table_name (a,b,c,x,y,z);

    not:

    INSERT INTO table_name (a,b,c,
x,y,z);

  • Spaces in names get replaced with an underscore. Hyphens are allowed. Most other symbols are removed from item/mob/npc names except for polutils_name or packet_name columns, where they must be escaped.

  • Full lower case skill/spell/pet/ability things.

  • Don't change SQL keywords to lowercase:

    INSERT INTO table_name

    not:

    insert into table_name

Commenting in SQL

Our SQL tables are big and confusing, and they are also modified by hand. It can be very helpful to leave short comments on your additions and modifications to highlight what they are.

Example

Without a comment, this entry is not easily human-readable:

INSERT INTO `mob_droplist` VALUES (504,0,0,1000,888,340);

So we instead store it as:

INSERT INTO `mob_droplist` VALUES (504,0,0,1000,888,340); -- (Colossal Calamari) seashell

Conversely, Combo weaponskill doesn't need any additional comments because it has a name field:

INSERT INTO `weapon_skills` VALUES (1,'combo',0x02020000000200000000000002000000000202000000,1,5,0,16,2000,5,1,8,0,0,0,0);

The format of the comment isn't massively important, but it is preferred not to use ';' as a seperator in the middle of your comment. This is a little confusing, as it's the statement-terminator in SQL.

SQL Migrations for Schema changes

  • Going forward schema changes should be accompanied by a migration script.