WittleWolfie / WW-Blueprint-Core

A library to simplify modifying Pathfinder: Wrath of the Righteous.
MIT License
15 stars 5 forks source link

Update AddBlindsense to require m_ExceptionFeatures #36

Closed pheonix99 closed 2 years ago

pheonix99 commented 2 years ago

It is exceedingly annoying to have to do trial-and-error debugging to figure out which logically inapplicable optional paramater in a method is actually mandatory.

For instance - the only way to know that the reason AddBlindsense actually does require the m_ExceptionFeatures field to be filled even with hasExceptions false is brute force.

This is moderately annoying with a four paramater method, and infuriating with larger ones.

Alternately, for a quick and dirty fix, make optional array paramaters instantiate empty arrays if not filled.

WittleWolfie commented 2 years ago

I'm working on improvements to this but it can't really be solved without manual tuning on a per-type basis.

The main change I want to implement is to improve fallback to allow default values from the source to be applied. This will probably address a lot of the cases but short of manually configuring each type there's no way to guarantee it is correct.

WittleWolfie commented 2 years ago

The reason for making parameters optional is to limit boilerplate, given that we don't know which fields are required and which are not. For some components it would be an absolute mess if you had to populate all fields because there might be 10 available and you only need to set 1 or 2.

WittleWolfie commented 2 years ago

Ugh, and Blindsense is actually a perfect example. If you just create it the default value of m_ExceptionFacts is null. I'm inclined to agree that by default populating empty arrays and lists is a good idea. I find it hard to imagine there are code paths that fail if there's an empty list but function with null.

WittleWolfie commented 2 years ago

@pheonix99 Getting closer to 2.0 release which should address this issue, insofar as Owlcat code handles things correctly.

In the refactor branch all parameters are defaulting to optional and will remain unset except for specific types which are overridden to have sane / non-null default. Basically it looks something like:

public void AddXComponent(object someObj = null, LocalizedString name = null)
{
  var component = new XComponent();
  component.m_SomeObj = someObj ?? component.m_SomeObj;
  component.m_Name = name ?? component.m_Name;
  if (component.m_Name is null)
  {
    // Prevents bad behavior w/ null LocalizedString types
    component.m_Name = Constants.Empty.String;
  }
}

That doesn't totally protect you but a lot of common cases will be covered and the library won't make anything worse than what already exists.

Additionally, if you work with a type and realize that there are required parameters or other implicit requirements on the input you can file and issue or just PR a JSON config:

{
  "TypeName": "AreaEntranceChange",
  "RequiredFields": [ "m_Location", "m_NewEntrance" ]
},

This should make it a lot easier to improve the API over time.

WittleWolfie commented 2 years ago

Fix is in 2.0.x.