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

Notification #999

Closed
wants to merge 5 commits into from
Closed

Notification #999

wants to merge 5 commits into from

Conversation

BrettMayson
Copy link
Contributor

When merged this pull request will:

  • Create a simple notify function that creates a small notification for mod and mission makers to use rather than using hint or write themselves

@jokoho48
Copy link
Member

jokoho48 commented Oct 3, 2018

👎 don't like the use of spawn in there. should use waitAndExecute and waitUntilAndExecute maybe instead?

@BrettMayson
Copy link
Contributor Author

BrettMayson commented Oct 3, 2018

I didn't use waitAndExecute since it has multiple blocks requiring sleep. This would require nesting would it not?

I thought the use of sleep 3 times would be better overall than nesting 3 times

@jokoho48
Copy link
Member

jokoho48 commented Oct 3, 2018

I would build it more like a message queue so that you don't need to wait multiple times for every notification.

so that you have a function that just enqueues the notification, and one that pulls one and draws it and after x amount of time hides it and recalls itself to check if the queue is empty and when then just stops. with that, you only have X "tasks" running at the same time and not a "task" for every notification.

@BrettMayson
Copy link
Contributor Author

Wouldn't that require a task running during the entire mission to check the queue? Where as this method has no background tasks running while there a no notifications.

Considering that these notifications would rarely be shown isn't this way better? Even though if 3 notifications are being shown, there are 3 tasks, there should rarely be 3 notifications.

@jokoho48
Copy link
Member

jokoho48 commented Oct 3, 2018

Wouldn't that require a task running during the entire mission to check the queue? Where as this method has no background tasks running while there a no notifications.

no, because the function is called when you add a new notification. the draw function/tasks are only running when there is currently a notification on screen or in Queue. but than only the max amount of notifications that can get shown at the same time. and not more.

@BrettMayson
Copy link
Contributor Author

the function is called when you add a new notification

ah I see, I hadn't considered that way

waitUntil {_index - GVAR(NotificationCurrentIndex) < GVAR(NotificationMax)};

private _position = _index % GVAR(NotificationMax);
private _safezoneX = safezoneW + safezoneX;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the right edge of the (middle) screen. Safezone is a misleading variable name.


// Create the 3 displays in the offscreen position

private _ctrlBackground = _display ctrlCreate ["RscPicture", 20];
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic variable should be a macro.

0.1 * safezoneW,
0.06 * safezoneH
];
_ctrlBackground ctrlSetText "#(argb,8,8,3)color(0,0,0,0.75)";
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should use ctrlSetBackgroundColor instead. This as picture control seems awkward to me at least.

_ctrlBackground ctrlSetText "#(argb,8,8,3)color(0,0,0,0.75)";
_ctrlBackground ctrlCommit 0;

private _ctrlTitle = _display ctrlCreate ["RscStructuredText", 10];
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic variable should be a macro.

_ctrlTitle ctrlSetTextColor _color;
_ctrlTitle ctrlCommit 0;

private _ctrlNotification = _display ctrlCreate ["RscStructuredText", 30];
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic variable should be a macro.


private _ctrlNotification = _display ctrlCreate ["RscStructuredText", 30];
_ctrlNotification ctrlSetPosition
[
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent formatting. I prefer L57 & L67, and that is the style used in CBA in most places.

private _posX = 0.9 * safezoneW + safezoneX;
_position set [0, _posX];
_x ctrlSetPosition _position;
_x ctrlCommit (uiNamespace getVariable [QGVAR(NotificationSpeed), 0.5]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these ui namespace? Doesn't settings put the variables in mission namespace? It seems weird to put this into ui namespace.

@commy2
Copy link
Contributor

commy2 commented Oct 3, 2018

Not happy about this being scheduled. Basically breaks if you save and load a savegame or have poor FPS in MP.

@commy2
Copy link
Contributor

commy2 commented Oct 3, 2018

This should be a controls group control template in config, so the script doesn't have to recreate the whole thing with slow sqf every time.

@dedmen
Copy link
Contributor

dedmen commented Dec 1, 2018

arma3_x64 2018-11-30 23-23-26-06
Screenshot from @commy2
It isn't of much use if it's in slack and not here ;)
The above obviously won't get merged like that

@BrettMayson
Copy link
Contributor Author

That's what it looks like?

Wildly different from my PC.

I am going to be rewriting this PR after my exams anyway.

@commy2
Copy link
Contributor

commy2 commented Dec 1, 2018

I have a counter proposal here: #1036, which is kind of a port from the ACE one. It's compatible with the BI Layout editor.

@BrettMayson BrettMayson closed this Feb 6, 2019
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.

5 participants