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

Sfinv: Add 'position[]' parameter to theming #1919

Closed
wants to merge 1 commit into from
Closed

Sfinv: Add 'position[]' parameter to theming #1919

wants to merge 1 commit into from

Conversation

paramat
Copy link
Contributor

@paramat paramat commented Oct 17, 2017

screenshot from 2017-10-17 06-39-17

Allows a sfinv formspec to be positioned. Useful for skin mods to allow seeing the player using F7 while selecting skins.
@rubenwardy

@rubenwardy
Copy link
Member

I think this is an engine bug. It shouldn't matter where you specify position really.

@paramat
Copy link
Contributor Author

paramat commented Oct 17, 2017

I tried placing it just before 'size[]' as that seemed to make more sense but it didn't work.
'position' seems to be something that belongs alongside 'size' in the theming parameters, it isn't intuitive anywhere else.

@rubenwardy
Copy link
Member

'position' seems to be something that belongs alongside 'size' in the theming parameters, it isn't intuitive anywhere else.

Size is there as all formspecs have a size. Position is just a control for where the formspec is - under that logic, background with auto clipping should also be a parameter as just like position it only works once. I'd prefer for position to be allowed anywhere, and the last one be the one to take effect, as it requires less specific code in sfinv. I believe position was added this dev cycle anyway. However, I'm not strongly opposed to this PR.

@paramat
Copy link
Contributor Author

paramat commented Oct 22, 2017

That makes sense, closing. reopening issue minetest/minetest#6538

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants