Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding new functions: NWNX_ON_PLACEABLE_OPEN_{OPEN|CLOSE}_{BEFORE|AFTER} #1691

Merged
merged 15 commits into from
Aug 22, 2023
Merged

Adding new functions: NWNX_ON_PLACEABLE_OPEN_{OPEN|CLOSE}_{BEFORE|AFTER} #1691

merged 15 commits into from
Aug 22, 2023

Conversation

ReachPW
Copy link
Contributor

@ReachPW ReachPW commented Aug 9, 2023

This adds new events for Placeables, when opening and closing inventory.

NWNX_ON_OBJECT_OPEN_BEFORE
NWNX_ON_OBJECT_OPEN_AFTER
NWNX_ON_OBJECT_CLOSE_BEFORE
NWNX_ON_OBJECT_CLOSE_AFTER

Copy link
Contributor

@Cjreek Cjreek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I also missed this in the first review. But you need to do another commit anyway:

@ReachPW
Copy link
Contributor Author

ReachPW commented Aug 11, 2023

Should I delete this PR and do a new "clean" PR with just the finalized changes?

Copy link
Member

@Daztek Daztek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since those events only run for placeables I think it's better to name the events NWNX_ON_PLACEABLE_{OPEN|CLOSE}_* or NWNX_ON_PLACEABLE_INVENTORY_{OPEN|CLOSE}_*

Also please fix whatever's going on with that SWIG postprocess.sh!

Comment on lines 138 to 141
auto PushAndSignal = [&](const std::string& ev) -> bool {
PushEventData("OBJECT", Utils::ObjectIDToString(thisPtr->m_idSelf));
return SignalEvent(ev, oidOpener);
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer SignalEvent being called with thisPtr->m_idSelf so OBJECT_SELF is the placeable, since that's most, if not all events do, and push the opener as event data, eg: PushEventData("OPENER", Utils::ObjectIDToString(oidOpener));

Same for the CloseInventory event

@ReachPW
Copy link
Contributor Author

ReachPW commented Aug 11, 2023

Since those events only run for placeables I think it's better to name the events NWNX_ON_PLACEABLE_{OPEN|CLOSE}* or NWNX_ON_PLACEABLE_INVENTORY{OPEN|CLOSE}_*

updated.

Also please fix whatever's going on with that SWIG postprocess.sh!

Sorry about that, but not sure what is going on here.

When I do a new pull of repo it looks fine.

cat Plugins/SWIG/SWIG_DotNET/postprocess.sh 
#!/bin/bash

#Cleanup macro comments left by SWIG.
for f in out/*.cs
do
  sed -i 's/\/\*@SWIG.*\*\///' "$f"
done

I tried a few things to remove it from PR entirely, but still showing as empty under Files.

It was "undeleted" in this change: https://github.com/ReachPW/unified/commit/88d6c18213fb6640ce24cd3646d3afe5107c4541

so should be same as before, but I still don't understand why it showing 'empty' in the PR.

I can delete this PR and do a fresh PR with just the finalized changes and excluding that postprocess.sh ? Otherwise not sure how to resolve/fix.

@Daztek Daztek changed the title Adding new functions: NWNX_ON_OBJECT_{OPEN|CLOSE}_{BEFORE|AFTER} Adding new functions: NWNX_ON_PLACEABLE_OPEN_{OPEN|CLOSE}_{BEFORE|AFTER} Aug 22, 2023
@Daztek Daztek merged commit d0ebd0f into nwnxee:master Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants